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

fix(grid): refactor imports to not cause de-optimization for esm bundlers #1298

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lukasholzer
Copy link

🚀 Enhancements

When importing from @visx/grid/lib/shapes/Line it forces bundlers to use the CommonJS version instead of the ECMAScript Module version which leads to larger bundle sizes due to lacking tree-shaking capabilities (CommonJS uses require statements).

If the types are not exposed from the root barrel file they should reside in their own import as type imports will be omitted.

… esm bundlers.

When importing from `@visx/grid/lib/shapes/Line` it forces bundlers to use the CommonJS version instead the ECMAScript Module version which leads to larger bundle sizes due to lacking tree-shaking capabilities (commonjs uses `require` statements).

If the types are not exposed from the root barrel file they should reside in their own import as type imports will be omitted.
@williaster
Copy link
Collaborator

Hey @lukasholzer 👋 thanks for the contribution! this is great, and we could probably update more than the @visx/grid package with this improvement.

looks like this syntax was introduced in TS 3.8 which is what we're on currently, but seems like eslint/prettier doesn't like it. so we may need to try and bump those configs. Our dev config is a little complicated, I can try and bump versions next week to see if it fixes this.

@williaster williaster changed the title Refactor imports to not cause de-optimization for esm bundlers fix(grid): refactor imports to not cause de-optimization for esm bundlers Jul 30, 2021
@williaster
Copy link
Collaborator

Hey @lukasholzer wanted to check back on this. Our TS version should now support this syntax.

@alj1s
Copy link

alj1s commented Sep 8, 2022

Hey @lukasholzer @williaster it would be great to see this MR merged. Is there anything outstanding that needs to be checked/tested that I could assist with?

@williaster
Copy link
Collaborator

hi @lukasholzer @ximenean happy to attempt a merge, CI is still failing so it needs to be rebased as a start

@alj1s
Copy link

alj1s commented Sep 20, 2022

@williaster looks like the rebase has no conflicts so no issues there
@lukasholzer I can't update this PR directly, so would you like to rebase and push an update or is it preferable for me to submit a new PR?

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.

None yet

3 participants