Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: typescript definition & allow mainPath opt. #79

Conversation

josheleonard
Copy link

@josheleonard josheleonard commented Mar 10, 2020

fixes: #65
&
fixes: #61
&
fixes: #87

This PR converts the project to use TypeScript and exposes the type definitions to consumers.
This also adds the mainFile property to the options argument, allowing apps to declare which "main" file they would like to watch.

appArgv option added to allow sending custom launch arguments to your app

electronArgv option added to pass launch arguments directly to electron

mainFile, appArgv, & electronArgv options usage example:

if (isDev) {
  const electronReload = require('electron-reload');
  electronReload(path.join(__dirname, '..'), {
    electron: path.join(__dirname, '../..', 'node_modules', '.bin', 'electron'),
    mainFile: path.join(__dirname, 'index.js'),
    hardResetMethod: 'exit', //
    appArgv: ['--my-custom-flag'],
    electronArgv: ['--enable-logging'],
  });
}

…ainFile-option

feat: typescript definition & allow mainPath opt.
typescript definitions and mainFile option added
allow package to be installed directly from repo
fix: fix imports and installation
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@yan-foto
Copy link
Owner

Thanks for this PR. TS has been a very popular requested feature! Yet, as I lack experience with TS, it might take me some time to have it merged.

@mackignacio
Copy link

mackignacio commented Mar 12, 2020

@yan-foto

Thanks for this PR. TS has been a very popular requested feature! Yet, as I lack experience with TS, it might take me some time to have it merged.

I think if you don't want to use typescript but want to have a typescript compatibility. You must have a type declaration file .d.ts only for the library. This way you can continue developing the library with JS and still be compatible with typescript user without changing the whole code base to typescript.

See PR #84 for generating the declaration file only.

@yan-foto
Copy link
Owner

I think if you don't want to use typescript but want to have a typescript compatibility. You must have a type declaration file .d.ts only for the library. This way you can continue developing the library with JS and still be compatible with typescript user without changing the whole code base to typescript.

@josheleonard I would prefer this solution and would really appreciate if you or @mackignacio could update this PR respectively. This way we also don't need a major update and a minor one would suffice.

I suppose the best way is to generate the types on the fly using npm's prepublish script. npmignore should also be updated to ignore any files that does not need to be pushed to npm repository.

See PR #84 for generating the declaration file only.

Its not a good idea to have two PRs doing the same thing!

@mackignacio
Copy link

@yan-foto Sorry for making another PR for the declaration files. I already updated it and added a prepublishOnly script on the package.json file. Do you want me to just use this PR and close #84?

Also reversed args for hard reset handler
- allows file-argument to be passed to electron in the correct order
- usecase: opening file in electron app from cli

fixes: yan-foto#65
&
fixes: yan-foto#61
This version fixes the following issues:
fixes: yan-foto#65
&
fixes: yan-foto#61
@josheleonard
Copy link
Author

@yan-foto @mackignacio See my latest commits top this PR. I have implemented my changes using @mackignacio 's PR as reference. I also swapped the concat order of the hard reset handler since files are typically the last launch argument when opening files to an electron app from cli. My use case for this is I have an app that is a file viewer, and can be opened by cli and passing in a file path to open/view.

This suppresses ts errors about the declaration file.

This release fixes:
fixes: yan-foto#65
&
fixes: yan-foto#61
@yan-foto
Copy link
Owner

@josheleonard sorry for the delayed review. These times are some tough times for me 🤕

Copy link
Owner

@yan-foto yan-foto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments, feel free to give feedback, correct or contradict me.

.npmignore Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated
"lint:fix": "standard --fix"
"lint": "./node_modules/.bin/standard",
"lint:fix": "./node_modules/.bin/standard --fix",
"build": "./node_modules/.bin/rimraf main.d.ts && npm run lint:fix && ./node_modules/.bin/tsc",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove rimraf please (see bellow). I suppose tsc would overwrite the file and if not there is surely a flag to do so.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually tsc will not overwrite the .d.ts as seen in this discussion microsoft/TypeScript#16749 . That is why I think he added the rimraf commands.

@yan-foto you can check my other PR for just adding declaration. It is simple and easy to implement. Also this PR tackles 2 issue. We can focus on the other issue with this PR and resolve the issue for typescript in my PR #84 .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it to work without rimraf by adding the .d.ts to the exclude rules

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you guys should work together. As might have already figured out, I am running very low on spare time so it's not feasible for me to attend similar PRs at the same time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yan-foto Is the anything else you want changed? If so, @mackignacio do you want me to add you as a collaborator for my branch? I need to have #61 fixed so I can code my "main" file in typescript, then point the "mainFile" to my compiled .js file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josheleonard do you still need help for issue #61?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mackignacio #61 should be fixed, feel free to commit any changes you feel are needed.

@josheleonard
Copy link
Author

@yan-foto The requested changes have been pushed

Copy link
Author

@josheleonard josheleonard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See latest commit for changes

Copy link
Owner

@yan-foto yan-foto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a minor change is withstanding. Otherwise we're ready to merge!

main.js Outdated Show resolved Hide resolved
main.js Show resolved Hide resolved
@yan-foto
Copy link
Owner

Sorry for late reviews/replies. I only have very limited time on weekends to take care of my private stuff (this project being one of them!).

I really appreciate your patience and engagement. 🥇

@josheleonard
Copy link
Author

New changes are up :)

main.js Outdated Show resolved Hide resolved
@eugene-sh
Copy link

Any updates on this?

@josheleonard
Copy link
Author

@eugene-sh This PR is awaiting review.

@mackignacio
Copy link

I think this library is not active anymore. We need a library that is more active that can be updated. Many users are needing this typescript support but this PR takes a long time to review.

@yan-foto
Copy link
Owner

yan-foto commented Jul 25, 2021

I think this library is not active anymore. We need a library that is more active that can be updated. Many users are needing this typescript support but this PR takes a long time to review.

Feel free to use alternatives. Both this PR and those provided by you (#64 / #84) do not pass the quality assurance; for details see the comments.

@yan-foto yan-foto closed this Jul 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Watching __dirname and not what I set path to Add option for file arguments Typescript Definition
5 participants