-
Notifications
You must be signed in to change notification settings - Fork 80
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
build: replace pika with esbuild and tsc #819
Conversation
I'm 👍 on esbuild and tsc, and this approach seems reasonable to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️🔥 Feedback
Amazing job @wolfy1339! Thanks for taking the time and effort on this. This will have a lot of benefits for the community and us 👏🏽 🥳
I checked your changes and ran diff
to compare both outputs. I see some differences, but I'm unsure if they are expected.
Node version used
: 14.21.3
npm version used
: 9.6.0
📦⚖️ Output differences
I see some differences which I'm not sure if they are expected or not:
pkg/LICENSE.md
missing .md
extension. Is it intended?
pkg/package.json
- The
author
appears with ESBuild. I think this is an improvement 👏🏽 - Fixed
files
patterns ✅ - With
pika
, themain
,module
,types
andsource
paths were not starting with a relative path (./
). Is this an intended change?
pkg/dist-node/index.js
and pkg/dist-web/index.js
I see noticeable file size differences:
Checking the differences, I see some variable naming differences and spacing/indentation, so I'm assuming ESBuild does a better job there. Still, the file size increases 19kb
to 21kb
, which I guess is something we don't want.
pkg/dist-src/*
We are emitting source maps, but I think we don't want the dist-src/
, right? I guess the non-source-map files increased their size because of sourcemaps.
pkg/dist-types
It looks identical to me 🍾 ✅
📊 Benchmarks
I leave here some screenshots of benchmarks I run on my local machine.
command: hyperfine --runs 3 'npm run build'
I don't have much experience with esbuild
but I was expecting better execution times. Maybe is tsc
consuming our time here? Or maybe the js
file invoking esbuild
? Since the time is almost the same this is something I would be happy to nerd about in a different PR 🤓 unless you see any easy quick-win here.
I would be happy to support you here. Let me know if you want to split the work or
Pika (main
branch)
ESBuild (esbuild
branch)
Migration Plan
Do you have any plan in mind for extending this to the rest of the repositories of the ecosystem? I'm happy to help with octoherd-scripts or any other approach we plan to do to migrate this.
This comment was marked as resolved.
This comment was marked as resolved.
With all changes since your review @oscard0m, the build time has reduced by 2 seconds.
That's 4 seconds faster than Pika
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Oscar Dominguez <[email protected]>
@wolfy1339 I resolved the conflicts. Are we ready to merge? To trigger a release, should the PR title be |
The releases are kinda screwed up. Since the It's not ideal, as we didn't have alpha releases enabled and the beta release was taken up by |
Is not possible to have 10.7.1 in |
Nope. You can only use a version number once, no matter the release channel. You can overwrite a release, but I don't think semantic release is capable |
Let's merge |
🎉 This issue has been resolved in version 11.0.0-beta.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 10.9.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 10.9.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pika has been deprecated for a while now, and the project has now been archived.
Uses esbuild to transpile the TS source code into an ESM source, NodeJs bundle, and a browser bundle
Uses the TypeScript compiler to generate the types
Behavior
Before the change?
After the change?
Other information
Additional info
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Type: Breaking change
label)If
Yes
, what's the impact:Pull request type
Please add the corresponding label for change this PR introduces:
Type: Maintenance