Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Only include built files in the NPM published package. #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .npmignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
node_modules
reports
npm-debug.log
coverage
.gitignore
.DS_Store
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
"version": "1.3.5",
"description": "AST utility module for statically analyzing JSX",
"main": "lib/index.js",
"files": [
"lib"
Copy link
Member

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.

Copy link

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… 😂

Copy link
Member

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.

Copy link

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. 😀

Copy link
Member

@ljharb ljharb Jan 12, 2017 •
edited
Loading

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".

Copy link

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.

Copy link
Member

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 :-/

Copy link

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.

],
"scripts": {
"build": "rimraf lib && babel src --out-dir lib",
"prepublish": "npm run lint && npm run test && npm run build",
Expand Down