-
-
Notifications
You must be signed in to change notification settings - Fork 32
Only include built files in the NPM published package. #19
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
Only include built files in the NPM published package. #19
Conversation
@@ -3,6 +3,9 @@ | |||
"version": "1.3.5", | |||
"description": "AST utility module for statically analyzing JSX", | |||
"main": "lib/index.js", | |||
"files": [ | |||
"lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I highly recommend using npmignore instead of the files array - it's much less dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically I recommend the files array for mostly the same reason… 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hazard with "npmignore" is that you'd accidentally publish unneeded files. The hazard with "files" is that you'd accidentally fail to publish a needed file.
An exclusion list is the safer approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️ I see where you're coming from but feel differently. Let's agree to disagree. 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but I'd really like to understand why you think "the package is too large" is worse than "the package code is broken in a way tests can't catch".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are important. I think you should ship the right files needed to make your package work, and nothing more. Test your compiled package before publishing if you think there's a chance you got it wrong (or make it part of your CI). Personally I've seen more cases of people screwing up npmignores than I have the files array which is part of why I lean for opt-in over opt-out.
I'm not saying your argument is wrong, we just have a different opinion and that's cool. This project already uses npmignore and so here I would recommend continuing to use that (unless the maintainers have an inclination to change). For my own projects and ones where I have an active role I would recommend using files array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to test the compiled package before publishing in a programmatic way, a project would need to add a test that runs npm pack
, untars the resulting tarball in a temp dir, cd's into it, and then what? If tests are published, it could run npm install && npm test
, but if tests are not published, how would the tests be able to run against that packed module?
The only way I can see this working is if all tests run npm pack
, untar, and import that to run tests against, ie, never running tests against the original source.
Can you point me to a single project that tests its published output? I can point to many projects that have screwed up what gets published such that the published package is unknowingly broken :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not proposing running a full test suite on your published output, just that your module is requireable. I realize that wasn't clear, sorry. I think requireability is the main issue if you think you didn't package it correctly (and mostly covers your last point) or some build step generated bad JS (eg babel somehow screws up or some gulp template is bad).
npm test
isn't going to test your actual packaged code if you're compiling, so I see little value in doing that, and like you said it would be pretty arduous to try.
Here's what I would do (I'd consider putting it in prepublish
too, though I wish that didn't run on npm install
):
npm pack
cd /tmp
npm install <path to pkg.tgz>
node -e "require('pkg'); require('pkg/other/entry'); " && echo "SUCCESS"
Anyway. Like I said, agree to disagree here. It's a super small detail and in the grand scheme, incredibly unimportant.
In response to #18
Sets files in the package.json to: lib
I tested the package with npm pack. The resulting package contained the following: