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

fix: exclude non-root README.md/LICENSE files #173

Merged
merged 8 commits into from
Aug 23, 2023

Conversation

AaronHamilton965
Copy link
Contributor

Summary

Currently, package publishers are not able to exclude non-root readme and license/licence files when publishing or packing their packages, as they are always strictly included.

This solution allows users to exclude non-root readme and license/licence files by excluding them in the files array inside package.json.

image

Added a new function called excludeNonRoot, which removes readme, license, or licence from the strictDefaults array, and adds the inverse of the file to the defaults array. This allows the root files to remain, while non roots are ignored.

Additionally, if users would like to, they can also exclude readme, license, or licence files from specific folders, for example: "!lib/**/README.md"

However, if users do not want to use this behavior, they can simply not include !readme, !license, or !licence and the original behavior will be preserved, meaning that ALL of those files, root or non-root, will be included.

Tap Test

npm test
All results pass

Dummy Project

Installed the npm cli repository, and added our changes to the npm-packlist’s index.js under node_modules.

Installed stylelint repository to have a dummy project for testing.
Make changes to package.json

alias localnpm="node /workspaces/cli/"
localnpm pack --dry

See which files are included/excluded

References

cli issue - 6341

@AaronHamilton965 AaronHamilton965 requested a review from a team as a code owner August 11, 2023 21:08
@ljharb

This comment was marked as off-topic.

@ljharb

This comment was marked as off-topic.

@rahulio96
Copy link
Contributor

rahulio96 commented Aug 11, 2023

This is for excluding non-root/nested readme and license files NOT top-level readme files, as those will always be included.

Sorry if the title was a little misleading, that's my fault. I was just trying to match it up with the original issue's title.

@ljharb

This comment was marked as resolved.

@ljharb
Copy link

ljharb commented Aug 11, 2023

Does this also work for .npmignore, not just files?

lib/index.js Outdated Show resolved Hide resolved
@rahulio96
Copy link
Contributor

We only tested it with the package.json files array, as it's what we thought the issue was calling for.

It likely does not work with .npmignore, as the function is being called inside another function that seems to deal with only package.json (processPackage)

@AaronHamilton965 AaronHamilton965 changed the title fix: exclude README.md/LICENSE files fix: exclude non-root README.md/LICENSE files Aug 11, 2023
@wraithgar
Copy link
Member

It looks like what we're doing is marking certain entries in strictDefaults as "all levels" or "root only".

Would it be possible for the new lines to be powered by a distinct set of values like strictDefaults and defaults, instead of three individual lines? Also, is there a reason that copying is not part of this new logic? If we say all of the entries in strictDefaults except .git is part of this new logic, can we refactor things to make it more clear? We'd have one for "all levels" and one for "root only"

Hope this makes sense. I think you're on the right track.

@ljharb
Copy link

ljharb commented Aug 11, 2023

It likely does not work with .npmignore, as the function is being called inside another function that seems to deal with only package.json (processPackage)

that implies that the bug also doesn't exist with npmignore, which would be a good thing :-)

@rahulio96
Copy link
Contributor

Hi again, sorry for the late reply, got caught up with something else.

Ok, I think I understand what you want us to do, and we'll be looking into it soon, just not sure about what you said here:

Also, is there a reason that copying is not part of this new logic?

@wraithgar
Copy link
Member

copying refers to

'!/copying{,.*[^~$]}',

@rahulio96

This comment was marked as outdated.

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
@rahulio96
Copy link
Contributor

@wraithgar Just saw the comment you left via email but can't find it on GitHub for some reason. Do you want us to bring back the strings to rootOnly or keep it as is?

@wraithgar
Copy link
Member

@rahulio96 I deleted my suggestion as you'd already implemented it and it wasn't a very strong opinion.

@wraithgar
Copy link
Member

wraithgar commented Aug 21, 2023

So far so good. I think it is potentially confusing having this work one way for files but not for .npmignore.

While not a deal breaker, I think that this at least deserves clarification in the README so folks know that you can't "unignore" readme (etc) files, but you can explicitly include them in the files array.

@rahulio96
Copy link
Contributor

Yeah I can get to work on updating the README description soon.

@wraithgar
Copy link
Member

Thanks! It'd be great to get this into npm 10 before it goes GA. We can make this a breaking change out of an abundance of caution and keep things tidy.

@wraithgar wraithgar merged commit 24344a2 into npm:main Aug 23, 2023
23 checks passed
@github-actions github-actions bot mentioned this pull request Aug 23, 2023
@rahulio96
Copy link
Contributor

Thank you both for your time, patience, and feedback, it was much appreciated!

@XhmikosR
Copy link

XhmikosR commented Sep 5, 2023

Could we maybe get this patch backported to the npm-packlist version that npm 9.x is using too?

@wraithgar
Copy link
Member

No this was considered a breaking change which is why it was held off till npm 10.

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