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

Use package.json files entry instead of .npmignore #5800

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Apr 15, 2022

Summary

🤔

#5799 ("remove .ci from the EUI package") got me exploring .npmingnore which reminded me of #4769 ("what else can we ignore?"). Reading the npm docs highlighted that we can make an include list rather than an exclude list by using the package.json files entry.

The result is a decrease of 8MB or ~11% of the distributed package.
Screen Shot 2022-04-15 at 10 15 36 AM

Confirmed Kibana and CRA both continue to work as expected.

Addendum
Yes, 63MB is still massive, but considering that includes 4 different targeted builds, 2 webpack bundles (one of which is 12MB), and 4 ~500KB CSS files. The CSS files will shrink rapidly with the move to CSS-in-JS, and we will likely stop distributing webpack bundles. Regardless, an actual consumer only (lol) sees 7MB (unpacked) in use, and the eventual move to packages will help immensely.

### Checklist

"test-env",
"eui.d.ts",
"i18ntokens.json",
"NOTICE.txt"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the main README as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for reminding me. There are certain files that are included regardless of your config; README is one of them: https://docs.npmjs.com/cli/v7/configuring-npm/package-json#files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh! 💡 TIL!

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this excellent cleanup!

we will likely stop distributing webpack bundles

Excited for this! 🎉

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5800/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!

@thompsongl thompsongl merged commit d6d841b into elastic:main Apr 15, 2022
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.

4 participants