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

chore: add files field for each package #3284

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

daydayhappychao
Copy link
Contributor

@daydayhappychao daydayhappychao commented Jul 31, 2023

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

There used to be a logic to sync .npmignore into the packages.

await fs.copy(path.resolve(BASE_DIR, '.npmignore'), path.resolve(dir, '.npmignore'));

I restored it and updated the content of .npmignore.

Please check if the content of .npmignore is reasonable. I'm not sure if the README.md should be excluded from packages.

@daydayhappychao daydayhappychao requested a review from a team as a code owner July 31, 2023 07:12
tools/gen-npmignore.ts Outdated Show resolved Hide resolved
@BlackHole1 BlackHole1 requested a review from a team July 31, 2023 09:10
@erickzhao
Copy link
Member

Thank you for this @daydayhappychao!!! I'll give this a review soon as well.

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Hi @daydayhappychao, thanks for the PR!

These days we generally prefer using "files" in package.json over .npmignore, and it's less likely to go stale. While this repo does have a .npmignore, it's quite old and as you found not being used since the top-level package.json isn't published and the other packages weren't using it.

Could this PR be refactored to add "files" to each package.json instead? Most of them will only need to include dist. Apologies for requesting extra work, but I think that solution would be better long-term.

@daydayhappychao
Copy link
Contributor Author

Hi @daydayhappychao, thanks for the PR!

These days we generally prefer using "files" in package.json over .npmignore, and it's less likely to go stale. While this repo does have a .npmignore, it's quite old and as you found not being used since the top-level package.json isn't published and the other packages weren't using it.

Could this PR be refactored to add "files" to each package.json instead? Most of them will only need to include dist. Apologies for requesting extra work, but I think that solution would be better long-term.

Great guide! I'll get this done soon. I agree that the files field is a more secure and reliable option.

@daydayhappychao
Copy link
Contributor Author

@erickzhao @BlackHole1 @dsanders11 Please review carefully, I feel like it's dangerous to get it wrong, even though I've double-checked.

@dsanders11 dsanders11 changed the title chore: sync .npmignore to each package before publishing chore: add files field for each package Aug 1, 2023
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

The files field changes LGTM so far, thanks for making the change. I've updated the PR title to match.

I believe we also need to include "src" for every package, sorry about that. @BlackHole1 pointed that out in an earlier comment but I forgot - there are some source maps in dist/ which refer back to the source files in src/ so we need to publish src/ as well.

@daydayhappychao
Copy link
Contributor Author

The files field changes LGTM so far, thanks for making the change. I've updated the PR title to match.

I believe we also need to include "src" for every package, sorry about that. @BlackHole1 pointed that out in an earlier comment but I forgot - there are some source maps in dist/ which refer back to the source files in src/ so we need to publish src/ as well.

Good idea. Pushing src/ into package is probably a better approach than building with inlineSource option.

It was a useful experience!

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @daydayhappychao!

@VerteDinde VerteDinde merged commit bdc9108 into electron:main Aug 8, 2023
3 checks passed
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.

5 participants