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

Rework bundler and bring back support to es2017 #1370

Closed

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Sep 3, 2022

Purpose

Rework the bundler config to ensure emitted file are valid es2017 files. Will fix #1362

PS: merged as part of #1408

Approach

Tried to keep on rollup, but easily extensible for esbuild

How to try it out

cd ./packages/rooks
yarn build
yarn check:dist
yarn check:size

Checks

Impact on size: negligible imo

image

Tasks

  • Emit es2017 thx typescript (could work with esbuild too)
  • Add module export that might help some bundlers (no changes)
  • Removed unused files (ie: jest.setup.ts, test files)
  • Removed declaration maps for dist (they are only useful in monorepo ?)
  • Removed dual types publishing (kept ./types/index.d.ts, removed per-file declarations)
  • Didn't understand what was the purpose of index.standalone, need to clear this
  • Merge rollup configs, and allow to build umd as well - To test it out
  • Post-cleanup: remove obsoleted deps...

@codesandbox
Copy link

codesandbox bot commented Sep 3, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@vercel
Copy link

vercel bot commented Sep 3, 2022

@belgattitude is attempting to deploy a commit to the Rooks Team on Vercel.

A member of the Team first needs to authorize it.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #1370 (8e0a59e) into main (c186858) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1370   +/-   ##
=======================================
  Coverage   91.61%   91.61%           
=======================================
  Files          76       76           
  Lines        4792     4792           
  Branches      816      816           
=======================================
  Hits         4390     4390           
  Misses        402      402           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

"exports": {
".": {
"types": "./dist/types/index.d.js",
"module": "./dist/esm/index.js",
Copy link
Contributor Author

@belgattitude belgattitude Sep 3, 2022

Choose a reason for hiding this comment

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

Kept the module for webpack support (it's not part of the final spec). Not sure if recent versions needs that.

Didn't add the browser field (umd), cause nextjs 12.x will pick it up and defeat tree-shakability (browser bundle should be one file for now)

Copy link
Owner

@imbhargav5 imbhargav5 Sep 4, 2022

Choose a reason for hiding this comment

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

@belgattitude Could you help me with any reading material for me to fully understand what has changed in the exports field? It changes so often I lost track of it. For eg, what does what field within exports do and what everyone else does, etc. Just want to make sure I understand all the changes we are making and we don't accidentally break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you feel unsure I can cherry-pick this and create another PR.

Copy link
Owner

Choose a reason for hiding this comment

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

@belgattitude Thanks for this! I will read this and get back. I will take a good look at this PR tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

TIL learnt a lot from that link today. Thanks!


const tsPathPlugin = typescriptPaths({
preserveExtensions: true,
tsConfigPath: "./tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typescriptPaths plugin does not support extend from tsconfig.build.json. So I use the paths from there. Note that it makes use remove composite: true.

@belgattitude
Copy link
Contributor Author

Hey @imbhargav5. It's just a start.

But I'd like to know

  • What's the purpose of publishing index.standalone.js ? Is it important ?
  • Are the declaration sourcempas and sourcemaps important ?

I'm not totally sure about this.

Happy week-end.

@imbhargav5
Copy link
Owner

Going to look at this tonight!

@imbhargav5
Copy link
Owner

@belgattitude index.standalone is an artefact from a year ago and we don't need that anymore. Thanks for pointing that out. We can remove it. 😄

@vercel
Copy link

vercel bot commented Sep 4, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
rooks ✅ Ready (Inspect) Visit Preview Sep 10, 2022 at 11:18AM (UTC)

@belgattitude
Copy link
Contributor Author

@imbhargav5

If all looks good to you, my plan is to replace the build with build-future and the new rollup config. I'll undraft when done.

My last concern, I saw a drop in size in the umd bundle. I guess is that the *.spec.ts aren't treated anymore. But I'd like another look.

@imbhargav5
Copy link
Owner

Will take a look.

@imbhargav5
Copy link
Owner

@belgattitude Looks like a great start. I am getting errors after I run yarn run build:future && yarn run check:dist. Is that expected?

@belgattitude
Copy link
Contributor Author

Yes it will check the ./dist/* folder, not the ./dist/future 😄 . I'll P/R tomorrow.

@imbhargav5 imbhargav5 marked this pull request as ready for review September 7, 2022 03:52
@belgattitude
Copy link
Contributor Author

Hey @imbhargav5 I've just finalized the thing.

Added

  • a (patch) changeset
  • a probably useful 'typecheck' step (no on CI though), that can pave the way for using esbuild.

Entered pre canary mode, so If you don't mind I'd like to test a canary version release first.

@belgattitude
Copy link
Contributor Author

Found a little time and fixed conflicts.

Ported the check-size in #1365

Good news, the size increase is very minimal

image

The build passed from 20s to 15s on my laptop. Imo it can be merged, let me know if you have questions.

Thanks for this beautiful lib

// @ts-check

// Regularly update this max size when new hooks are added (keep a threshold)
const fullBundleMaxSize = "12KB";
Copy link
Contributor Author

@belgattitude belgattitude Sep 8, 2022

Choose a reason for hiding this comment

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

@imbhargav5 just to mention, if enforced on CI and new hooks are added and the total size of lib grow, this should be increased.

On the other side, it's totally possible to keep only the "ESM Webpack ({ useDebounce })" (see below), that's actually the important check

Copy link
Owner

Choose a reason for hiding this comment

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

@belgattitude Ah. I think we could write a small script to autopopulate new hooks into the size-limit file when new hooks are added and keep the limit at 2kb. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea

@@ -7,6 +7,15 @@
"unpkg": "dist/umd/rooks.umd.js",
"types": "dist/types/index.d.ts",
"typings": "dist/types/index.d.ts",
"exports": {
".": {
"types": "./dist/types/index.d.js",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"types": "./dist/types/index.d.js",
"types": "./dist/types/index.d.ts",

@imbhargav5
Copy link
Owner

imbhargav5 commented Sep 11, 2022

This is such a massive improvement @belgattitude ! I just want to keep this in pre mode for a bit for a couple of days. Can we re-enter pre mode again? Once the conflicts are merged and my code suggestion resolved, I will get this shipped today. 🙌

@imbhargav5
Copy link
Owner

@belgattitude I continued your work here. #1408

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.

[V7] Bundled umd does not pass es2015-es2019 checks
2 participants