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

Bun build & pure ESM #157

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Bun build & pure ESM #157

wants to merge 17 commits into from

Conversation

coopbri
Copy link
Contributor

@coopbri coopbri commented Mar 20, 2025

Description

Task link: https://linear.app/omnidev/issue/OMNI-172/migrate-from-tsup-to-bun-build-and-remove-cjs-support
  • Converted from tsup to Bun build for bundling (tsup still used for type declaration generation)
  • Converted Sigil to ESM-only (removed CJS support)
  • Converted from binary to text-based Bun lock file
  • Upgraded ESLint from v8 to v9 (will migrate to dprint/Biome later)

The bundle should now be much smaller and build faster with the combination of general bundling optimizations (thanks to Bun) and dropping CJS artifacts.

Test Steps

  • Locally, test the build and make sure it's smol:

20250321_15h03m02s_grim

  • Important: test the build in a local package, make sure the styles appear properly from props
  • Offer feedback on further tweaks to Bun build configuration

If you have ideas on how to remove tsup, feel free to play around with it. Ideally, we'd use TypeScript isolated declarations, e.g. with the bun-plugin-dtsx plugin or tsc directly, but I didn't want to spend too much time on this and created https://linear.app/omnidev/issue/OMNI-242/use-isolated-declarations-for-type-output-and-remove-tsup

Even if you get it working, I'd rather do that in a separate PR, that way we can publish this version as ESM only without completely changing the build structure and minimize the blast radius a bit.

Copy link

changeset-bot bot commented Mar 20, 2025

🦋 Changeset detected

Latest commit: 08dee35

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@omnidev/sigil Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Mar 20, 2025

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

Name Status Preview Comments Updated (UTC)
sigil-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 22, 2025 11:23pm

@coopbri coopbri mentioned this pull request Mar 20, 2025
await Bun.build({
entrypoints: ["src/index.ts"],
outdir: "build",
external: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tsup automatically marks peer deps as external (see https://tsup.egoist.dev/#excluding-packages), Bun build does not. This is nicer since it's more explicit anyways

"main": "build/sigil.js",
"module": "build/sigil.mjs",
"types": "build/sigil.d.ts",
"type": "module",
Copy link
Contributor Author

@coopbri coopbri Mar 20, 2025

Choose a reason for hiding this comment

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

I considered adding "sideEffects": false, but I don't think that works for our situation as we have e.g. CSS imports in main entry point. Note sideEffects is boolean | string[]; if passed an array of file paths it will mark them as files with side effects. We could do this, but I figured this was a better optimization for the future since the build system is/might be affected quite a bit, e.g. we might want to split the library into a single entry point per file in the future (further discussion and research to be had). In fact, we probably should set sideEffects sooner than later, and that kind of refinement would be a good time to do it. https://linear.app/omnidev/issue/OMNI-245/set-sideeffects

Consequentially I decided to let downstream frameworks/bundlers handle the tree-shaking as we were doing before.

Copy link

pkg-pr-new bot commented Mar 21, 2025

Open in Stackblitz

npm i https://pkg.pr.new/omnidotdev/sigil/@omnidev/sigil@157

commit: 08dee35

Copy link
Contributor Author

@coopbri coopbri Mar 21, 2025

Choose a reason for hiding this comment

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

Don't worry about reviewing this file, it was generated by the ESLint migration CLI and I only modified it to get it working. Since we are migrating to dprint/Biome later anyways, it's not a big deal, and the rules match what we had before (with the exception of import/no-cycle disabled).

@coopbri coopbri marked this pull request as ready for review March 21, 2025 20:07
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.

1 participant