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

Optimize the npm package size #6105

Merged
merged 4 commits into from
Mar 3, 2019

Conversation

simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Mar 2, 2019

Explicitly target files that should be included in the npm package:

bower.json
composer.json
package.json
LICENSE.md
README.md
dist/*.css
dist/*.js

The following files will be removed:

dist/docs/*
docs/*
sample/*
scripts/*
test/*
src/*
.editorconfig
.eslintignore
rollup.config.js
... + other useless root files ...

I don't think anyone read the docs or browse samples from the node_modes/chart.js directory but instead access our website and/or GH projet. Removing these resources is not a breaking change so if someone complain about these missing files, we can still decide or not to re-introduce part of them.

Please review carefully files included in 2.7.3 and tell me if you think we should include something else.

Stats:

master PR Ratio
package size 3.4 MB 343 KB ~10x
unpacked size 9.0 MB 1.3 MB ~7x
total files 681 11 ~56x

Explicitly target files that should be included in the npm package, which make the npm package 8x smaller by removing the docs, samples, scripts, tests and other useless files.
@simonbrunel
Copy link
Member Author

I'm realizing that since #6048, it's not possible anymore for an external node project to import files directly from chart.js/src/*. This is because of this CSS import which requires a custom rollup build.

So we should either (1) remove the rollup step and duplicate the CSS code in the .js file, which would allow npm consumers to include our source files but also means we should not switch to ES6 imports until v3, or (2) remove the src folder from the npm package, which would make the package even lighter.

Not sure what solution (1) or (2) will be preferred by our users, neither if (2) would break any integrations?

@benmccann
Copy link
Contributor

You mentioned a few files in your comment that weren't whitelisted in the PR. I guess those get pulled in automatically?

  • package.json
  • LICENSE.md
  • README.md

It might be nice to copy src/platforms/platform.dom.css to dist/chartjs.css so that if folks use disableCSSInjection it's easier for them to combine our CSS with other CSS files in a build step in order to serve a single CSS file. I think that could be a good change to make with either solution. Assuming that change is made then I would prefer (2) as solution

@simonbrunel
Copy link
Member Author

I guess those get pulled in automatically

Right (npm docs):

Certain files are always included, regardless of settings:

- package.json
- README
- CHANGES / CHANGELOG / HISTORY
- LICENSE / LICENCE
- NOTICE
- The file in the “main” field

README, CHANGES, LICENSE & NOTICE can have any case and extension.

It might be nice to copy src/platforms/platform.dom.css to dist/chartjs.css

That's already the case, the build process generates Chart.css and Chart.min.css in the dist folder.

@simonbrunel simonbrunel mentioned this pull request Mar 2, 2019
4 tasks
@simonbrunel
Copy link
Member Author

I also prefer solution (2) because I don't think users should use our source files directly, but unfortunately, I can find a few cases where it's promoted to import src/chart.js (e.g. valor-software/ng2-charts @valorkin).

@etimberg
Copy link
Member

etimberg commented Mar 2, 2019

I like 2, but it's definitely breaking

@simonbrunel
Copy link
Member Author

simonbrunel commented Mar 2, 2019

Though, it doesn't seem to work (valor-software/ng2-charts#1041) so maybe we shouldn't try to support importing from our src folder, especially if we are going to convert to ESM imports? In this case we could go for (2).

If we really care to not break these projects that import node_module/chart.js/src/chart.js, we could rename this file to index.js and create an alias from src/chart.js to dist/Chart.js, something like:

// src/chart.js
module.exports = require('../dist/Chart.js');

It would only break projects that require other files (e.g. 'chart.js/src/helpers/helpers.core.js'), but do we really consider that as a supported scenario? To me, everything under src/* should be considered private because we should be able to move/rename/delete any files without considering a breaking change.

@kurkle
Copy link
Member

kurkle commented Mar 2, 2019

I like option 2 too. Maybe it should be include for compatibility for now and instruct people to import from dist?

@benmccann
Copy link
Contributor

I consider accessing src/ files directly the same as using @private members. Users can technically do it, but we're under no obligation to support it and they should expect it can break if they do it

kurkle
kurkle previously approved these changes Mar 2, 2019
@kurkle
Copy link
Member

kurkle commented Mar 2, 2019

I think we should do the rc with this, and see if anything needs to be changes?

@simonbrunel
Copy link
Member Author

I agree with @benmccann, so instead, should we go for (2) (remove src from the npm package) and create an alias to avoid breaking too many projects that misused our sources?

@simonbrunel
Copy link
Member Author

simonbrunel commented Mar 2, 2019

I removed the sources from the npm package and created an alias from src/chart.js to dist/Chart.js, the new published package will be 10x smaller than currently (stats updated).

I think the alias is useless for ng2-charts because after digging more, most examples use the dist files. Angular projects still using src/chart.js display an error message, whatever the Chart.js version. I'm not sure how to test this alias but would maybe keep it for other kind of node projects.

> npm publish --dry-run
package: chart.js@2.7.3
=== Tarball Contents ===
1.8kB   package.json
329B    bower.json
574B    composer.json
1.1kB   LICENSE.md
2.2kB   README.md
555.4kB dist/Chart.bundle.js
208.2kB dist/Chart.bundle.min.js
858B    dist/Chart.css
408.3kB dist/Chart.js
521B    dist/Chart.min.css
156.5kB dist/Chart.min.js
=== Tarball Details ===
name:          chart.js
version:       2.7.3
package size:  343 kB
unpacked size: 1.3 MB
total files:   11

etimberg
etimberg previously approved these changes Mar 2, 2019
kurkle
kurkle previously approved these changes Mar 2, 2019
@simonbrunel simonbrunel dismissed stale reviews from kurkle and etimberg via f36d410 March 2, 2019 19:29
@simonbrunel
Copy link
Member Author

@etimberg @kurkle I removed the alias because it will not help ng2-charts projects and until 2.8, the package.json main entry point was src/chart.js so there was no reason to explicitly require that file.

I might have missed some use cases?

@valorkin
Copy link

valorkin commented Mar 2, 2019

If you do esm, ng2 charts will be properly updated

@simonbrunel
Copy link
Member Author

@valorkin ng2-charts should use and promote dist/Chart.*.js (UMD builds)

@valorkin
Copy link

valorkin commented Mar 2, 2019

ng2-charts doesn't actually have a dependency on charts.js file, only docs has (the line you have pointed)
Lib itself expects charts to be globaly available

@simonbrunel
Copy link
Member Author

@valorkin it could be a good idea to update the documentation (README.md) because the suggested snippet is not correct and is reported multiple time not working. You should actually revert valor-software/ng2-charts#356 (see this comment).

@simonbrunel simonbrunel merged commit 35273ee into chartjs:master Mar 3, 2019
@simonbrunel simonbrunel deleted the chore/npm-files branch March 3, 2019 14:19
@simonbrunel simonbrunel mentioned this pull request Mar 15, 2019
3 tasks
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Explicitly target files that should be included in the npm package, making it 10x smaller by removing the docs, samples, scripts, sources, tests and other useless files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants