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

Move rules about release files from .gitattributes to package.json #1573

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

lfdebrux
Copy link
Member

@lfdebrux lfdebrux commented Sep 2, 2022

We're moving to releasing the kit as a package rather than as an archive of the repo, so we need to tell npm what should go into that package. This commit does that by adding a files list to package.json [1].

The advantage of using the files list is that npm will also use the rules in the .gitignore, so we don't need to worry about duplicating them, whereas if we used the alternative approach, a .npmignore file, then we would have to copy the rules from .gitignore into there. We can also add the .gitignore file to the files to pack and still have the rules be applied, which is good because we want to copy the .gitignore when making a new prototype from the starter files. The downside is that we do have to remember to add files to the files list in future, and there doesn't appear to be a way to ignore the *.test.js files.

On balance though I think that the *.test.js files are small, and the risk of accidentally including files in the release is greater than the downside of having these extre files. I find that having an allowlist like this is easier to understand than a denylist like the .npmignore method, so I've gone with the files list approach.

We're moving to releasing the kit as a package rather than as an archive
of the repo, so we need to tell npm what should go into that package.
This commit does that by adding a `files` list to `package.json` [[1]].

The advantage of using the `files` list is that `npm` will also use the
rules in the `.gitignore`, so we don't need to worry about duplicating
them, whereas if we used the alternative approach, a `.npmignore` file,
then we would have to copy the rules from `.gitignore` into there. We
can also add the `.gitignore` file to the files to pack and still have
the rules be applied, which is good because we want to copy the
`.gitignore` when making a new prototype from the starter files. The
downside is that we do have to remember to add files to the `files` list
in future, and there doesn't appear to be a way to ignore the
`*.test.js` files.

On balance though I think that the `*.test.js` files are small, and the
risk of accidentally including files in the release is greater than the
downside of having these extre files. I find that having an allowlist
like this is easier to understand than a denylist like the `.npmignore`
method, so I've gone with the `files` list approach.

[1]: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#files
Partially reverts changes in 664ad5 (Make prototype kit cli work outside
kit). In that commit we had to stop trying to copy the .gitignore file
because it wasn't included in the npm package. However, with the
`package.json` `files` list we can make sure that `.gitignore` gets
copied, so we can keep the copy and avoid the effort of duplication.
@lfdebrux lfdebrux mentioned this pull request Sep 2, 2022
3 tasks
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS left a comment

Choose a reason for hiding this comment

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

LGTM

@lfdebrux lfdebrux merged commit 6902529 into v13 Sep 2, 2022
@lfdebrux lfdebrux deleted the ldeb-npm-pack branch September 2, 2022 13:03
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.

2 participants