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

Add definition files #64

Closed
wants to merge 11 commits into from

Conversation

mackignacio
Copy link

Added definition file for typescript users.

@mackignacio
Copy link
Author

mackignacio commented Aug 3, 2019

@yan-foto I also updated chokidar to 3.0.2.

@yan-foto
Copy link
Owner

yan-foto commented Aug 5, 2019

@yan-foto I also updated chokidar to 3.0.2.

Version 1.5.0 includes the upgrade to chokidar v3.

@yan-foto
Copy link
Owner

yan-foto commented Aug 5, 2019

I need to take a closer look to this. I prefer users not to compile ts files into js when they install electron-reload. Typescript files should only be for development and be excluded from final node package (using .nodeignore).

Since I'm not familiar enough with TS, merging this PR might take a while. But I'll do it for sure! I think this is an important contribution.

@mackignacio
Copy link
Author

mackignacio commented Aug 5, 2019

They don't need to compile TS when they are installing it via npm i. When we publish electron-reload we will first compile TS then add the main.d.ts file on our npm package. The only thing we need to add on our npm package is the .d.ts file. We can add the main.ts on npmignore file.

Update: I push a commit with main.ts added in npmignore file.

@mackignacio
Copy link
Author

@yan-foto This is a sample npm package with declaration on it.

image

The index.d.ts is the declaration file ( also what they call typescript definition). This will be generated before running npm publish. You can run npm run tsc to generate that .d.ts file before publishing.

image

@yan-foto
Copy link
Owner

yan-foto commented Aug 6, 2019

This is still not the optimal case! I quickly searched for the best way to publish TS packages on NPM and found this nice piece of advice. Simple but elegant!

@mackignacio
Copy link
Author

mackignacio commented Aug 6, 2019

That will only works if we have ./src folder as shown below.
image
Also this code "tsc -p ./ --outDir dist/" will generate another main.js file that will overwrite existing main.js. Its assume you are working on a TS project not a JS one. This script tsc --declaration --emitDeclarationOnly main.ts will only just generate the .d.ts and not overwrite your existing js files.
We can add a prePublishOnly script like this just like what the article says.
image
But if you want this approach we will make our development 100% TS.

@yan-foto
Copy link
Owner

yan-foto commented Aug 7, 2019

That was my understanding from your PR that this project is now pure TS, as you renamed main.js to main.ts and adapted the content. Moreover you don't need to follow that article line by line, but to implement the gist of it. Since we only have one source file, we don't need a separate src file and from my understanding tsc can generate the type definitions file. So at the end we will end up with main.ts, a prePublish script that generates the type definitions and transpiles the main file, and adapted .nodeignore and .gitignore to keep both npm and github clean.

That would be the perfect case in my opinion.

@mackignacio
Copy link
Author

Ok got it. I will make this PR a 100% TS. I will remove the main.js and generate it on prepublish script to make it cleaner.

@mackignacio
Copy link
Author

Updated the main.ts with the current main.js code. Removed main.js and generate it on prepublish. @yan-foto you can check if there are some more adjustments to be made.

@yan-foto
Copy link
Owner

Perfect. Please give me some time to review it, before I can merge it.

@mackignacio
Copy link
Author

LGTM

@yan-foto
Copy link
Owner

Sorry for late reply. This PR has some major issues, which costed me long enough to fix so that I will suspend it. Here's a list:

  • Soft reset is broken because a crucial electron callback was removed (browserWindows remains empty)
  • tsc returns errors regarding Symbol and Object.assign (due to wrong es5 target)
  • Built files are mixed with source files in the root and are not excluded in .gitignore
  • There is no proper typing for option objects (and no, any is not good enough!)

To avoid any further possible breakage, I will close this PR.

@yan-foto yan-foto closed this Sep 14, 2019
@mackignacio
Copy link
Author

Thank you for your good insight. I will try to make another PR with the fixes on the issue you point out.

@mackignacio mackignacio deleted the add-definition-files branch March 12, 2020 04:16
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.

None yet

2 participants