Skip to content

Conversation

favna
Copy link

@favna favna commented Jun 1, 2019

Purpose / Goal

I noticed that the version published to NPM has some remaining unnecessary files. This is sort of the issue when using npmignore as you're specifying which files not to publish which requires maintaining it. Instead it's far better to use the files property in package.json where you specify which files you do want.

Shown below are the before and after of npm publish --dry-run. As you can see just removing the remaining files ensures a times 5 decrease of bundle size (from 106.7 kB to 22.1 kB!), primarily due to filtering out the lockfile which does nothing for NPM installs.

Before:

$ npm publish --dry-run
npm notice
npm notice �  fast-xml-parser@3.12.16
npm notice === Tarball Contents ===
npm notice 2.3kB   package.json
npm notice 1B      BACKERS.md
npm notice 10.4kB  CHANGELOG.md
npm notice 3.0kB   cli.js
npm notice 2.6kB   CONTRIBUTING.md
npm notice 1.3kB   LICENSE
npm notice 462B    nexttodo.md
npm notice 19.6kB  README.md
npm notice 219.7kB yarn.lock
npm notice 7.1kB   src/json2xml.js
npm notice 3.8kB   src/nimndata.js
npm notice 1.8kB   src/node2json_str.js
npm notice 1.1kB   src/node2json.js
npm notice 1.9kB   src/parser.d.ts
npm notice 925B    src/parser.js
npm notice 2.4kB   src/read.js
npm notice 1.9kB   src/util.js
npm notice 9.7kB   src/validator.js
npm notice 444B    src/xmlNode.js
npm notice 7.8kB   src/xmlstr2xmlnode.js
npm notice 909B    tasks/postinstall.js
npm notice === Tarball Details ===
npm notice name:          fast-xml-parser
npm notice version:       3.12.16
npm notice package size:  106.7 kB
npm notice unpacked size: 299.3 kB
npm notice shasum:        2e9c749186a8c799cd954cb7336457ef8663a9ca
npm notice integrity:     sha512-szo5Ha7mPQ78I[...]hG8qviuM+2HgQ==
npm notice total files:   21
npm notice
+ fast-xml-parser@3.12.16

After:

npm publish --dry-run
npm notice
npm notice �  fast-xml-parser@3.12.16
npm notice === Tarball Contents ===
npm notice 2.4kB  package.json
npm notice 1B     BACKERS.md
npm notice 10.4kB CHANGELOG.md
npm notice 3.0kB  cli.js
npm notice 1.3kB  LICENSE
npm notice 19.6kB README.md
npm notice 7.1kB  src/json2xml.js
npm notice 3.8kB  src/nimndata.js
npm notice 1.8kB  src/node2json_str.js
npm notice 1.1kB  src/node2json.js
npm notice 1.9kB  src/parser.d.ts
npm notice 925B   src/parser.js
npm notice 2.4kB  src/read.js
npm notice 1.9kB  src/util.js
npm notice 9.7kB  src/validator.js
npm notice 444B   src/xmlNode.js
npm notice 7.8kB  src/xmlstr2xmlnode.js
npm notice === Tarball Details ===
npm notice name:          fast-xml-parser
npm notice version:       3.12.16
npm notice package size:  22.1 kB
npm notice unpacked size: 75.7 kB
npm notice shasum:        03cab79e560b65bf6a9787843c821650de356a4d
npm notice integrity:     sha512-a66ah8hTcPfMF[...]9fJOCq7/1DhPA==
npm notice total files:   17
npm notice
+ fast-xml-parser@3.12.16

Type

Please mention the type of PR

  • [ ]Bug Fix
  • [x]Refactoring / Technology upgrade
  • [ ]New Feature

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.198% when pulling 3e9bf82 on Favna:master into 8a090d7 on NaturalIntelligence:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.198% when pulling 3e9bf82 on Favna:master into 8a090d7 on NaturalIntelligence:master.

@amitguptagwl
Copy link
Member

In my understanding, including lock file increase the bundle size but reduce the installation time. So need to confirm if this is really helpful not to publish them. Moreover, in my opinion, bundle size matters when it is published to be used as a web version.

@favna
Copy link
Author

favna commented Jun 1, 2019

First of all, if any lockfile is going to be included it should be the npm package-lock as the majority of the Node community uses npm and not yarn. I pray you don't need me to link half a dozen statistic tracking sites to back this as it's common sense.

Secondly, the npm docs only state that the package lock should be included in version control (git) and not in the dist bundle (see: https://docs.npmjs.com/files/package-locks#using-locked-packages). There is no information at all on it "speeding up installs", not even for development environments. I have no idea where you got this information.

Thirdly, if including the lockfile would improve end-user experience then wouldn't you think that big packages would include it? Don't answer that - they would. But the contrary couldn't be more true. By example here are screenshots for React, Angular and Moment:

ImageImageImage

And lastly, while it's true that bundle sizes matter most for browser bundles there is still no reason to bloat the node_modules folder for Node users. You wouldn't put a .psd file you used for making logos in the dist either.

@amitguptagwl
Copy link
Member

I downloaded the published code and found hat it has only necessary files only. yarn.lock has been set not to be published from next release.

Hence, I'm closing this PR. Thanks for the efforts you made.

@favna
Copy link
Author

favna commented Nov 16, 2019

@amitguptagwl that would be because you guys added an npmignore file. However npmignore files are extremely suboptimal because the moment you add another file that is not including in the exclusions it gets added to the published version. Instead you should use the files field in package.json as I did with this PR and rather than specifying which files to ignore, specify which files to include - which is 99/100 times a way smaller amount.

@amitguptagwl
Copy link
Member

amitguptagwl commented Nov 17, 2019 via email

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.

3 participants