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

build: replace pika with esbuild and tsc #819

Closed
wants to merge 27 commits into from
Closed

build: replace pika with esbuild and tsc #819

wants to merge 27 commits into from

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented Mar 6, 2023

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?

  • Uses pika for the build system

After the change?

  • Uses esbuild to generate the JS bundles
  • Uses the TypeScript compiler to generate the type definitions

Other information


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Dependencies/code cleanup: Type: Maintenance

@wolfy1339 wolfy1339 added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Mar 6, 2023
@kfcampbell
Copy link
Member

kfcampbell commented Mar 10, 2023

I'm 👍 on esbuild and tsc, and this approach seems reasonable to me.

@wolfy1339 wolfy1339 marked this pull request as ready for review March 10, 2023 23:07
Copy link
Member

@oscard0m oscard0m left a 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, the main, module, types and source 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:
image

image

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 ⚠️ from 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.
image

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)

image

ESBuild (esbuild branch)

image

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.

scripts/build.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
scripts/build.ts Outdated Show resolved Hide resolved
scripts/build.ts Outdated Show resolved Hide resolved
scripts/build.ts Outdated Show resolved Hide resolved
@wolfy1339

This comment was marked as resolved.

scripts/build.ts Outdated Show resolved Hide resolved
scripts/build.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@wolfy1339
Copy link
Member Author

wolfy1339 commented Mar 14, 2023

With all changes since your review @oscard0m, the build time has reduced by 2 seconds.

$ hyperfine --runs 3 'npm run build'
Benchmark 1: npm run build
  Time (mean ± σ):      7.026 s ±  0.204 s    [User: 14.463 s, System: 0.413 s]
  Range (min … max):    6.864 s …  7.255 s    3 runs

That's 4 seconds faster than Pika

$ node --version
v18.14.0
$ npm --version
9.3.1

@wolfy1339

This comment was marked as resolved.

@ollie-iterators

This comment was marked as outdated.

@wolfy1339

This comment was marked as outdated.

@oscard0m

This comment was marked as outdated.

@oscard0m

This comment was marked as resolved.

scripts/build.mjs Outdated Show resolved Hide resolved
scripts/build.mjs Outdated Show resolved Hide resolved
scripts/build.mjs Show resolved Hide resolved
scripts/build.mjs Outdated Show resolved Hide resolved
Co-authored-by: Oscar Dominguez <dominguez.celada@gmail.com>
@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

@wolfy1339 I resolved the conflicts. Are we ready to merge? To trigger a release, should the PR title be fix instead?

@wolfy1339
Copy link
Member Author

The releases are kinda screwed up.

Since the next branch has a few releases that take up some patch versions, we'll need to merge that into main for it to trigger a proper release.

It's not ideal, as we didn't have alpha releases enabled and the beta release was taken up by v11

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

The releases are kinda screwed up.

Since the next branch has a few releases that take up some patch versions, we'll need to merge that into main for it to trigger a proper release.

It's not ideal, as we didn't have alpha releases enabled and the beta release was taken up by v11

Is not possible to have 10.7.1 in latest even we are in 10.7.3 in next?

@wolfy1339
Copy link
Member Author

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

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

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 next in main then. The release notes look decent to me.

@wolfy1339 wolfy1339 mentioned this pull request Apr 10, 2023
@wolfy1339 wolfy1339 deleted the esbuild branch April 12, 2023 21:09
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 11.0.0-beta.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 10.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This issue has been resolved in version 10.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @beta released on @10.x released Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants