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 System: added rollup to produce es2015 & es5 imports, flattened types file #199

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

Conversation

ScottAwesome
Copy link

This PR adds rollup as the build system and adds DTS rollup via rollup-plugin-dts. This build will produce both es2015 and es5 builds.

I have also modified the package.json to utilize the new exports syntax to make import paths easier. This is compatible all the way back to Node 12 (and its ignored by older versions beyond that). Rollup, vite, parcel and webpack all understand the exports conditions by default as well.

I figured this might be a major version bump, since it slightly changes the import semantics, so I also bumped this to 2.0.0

Thanks!

@ScottAwesome ScottAwesome requested a review from Skn0tt as a code owner September 5, 2022 22:06
@Skn0tt
Copy link
Collaborator

Skn0tt commented Sep 5, 2022

Hi @ScottAwesome! I‘m currently on vacation and will only be able to take a closer look in a couple of weeks - only took a very shallow look from my phone for now.

I‘m wondering what benefits arise from these changes. It‘s in the project‘s interest to stay as easy to maintain as possible, so I‘d like to be careful with adding complexity.

Could you outline the issues with the current build system, and how this PR addresses them?

@ScottAwesome
Copy link
Author

ScottAwesome commented Sep 6, 2022

Hi @ScottAwesome! I‘m currently on vacation and will only be able to take a closer look in a couple of weeks - only took a very shallow look from my phone for now.

I‘m wondering what benefits arise from these changes. It‘s in the project‘s interest to stay as easy to maintain as possible, so I‘d like to be careful with adding complexity.

Could you outline the issues with the current build system, and how this PR addresses them?

Sure I'd be happy to elaborate! I understand it may ultimately not be a good fit.

That said, here's the limitations I feel building with raw tsc has

  1. No declaration file rollup. As a consumer, this prevents me from accidentally import from an internal API while still exposing the types for the purpose of understanding the library, but preserves API contracts

  2. The current output is spread across the same file structure (across dist and dist/esm) as src which, while not an insurmountable thing, does make it possible to consume parts of the package that I would imagine aren't considered part of the public API.

    • (Opinion) I find it (and some of co-workers too, which prompted this PR in part) easier to read source code when its a flat file to understand what its doing and what its underlying API contract is. The tsc output being spread across can be a hindrance to this. Not major, but is one of the motivating factors for me
  3. Output different syntax targets is more complex. You'd not only have to build dist, dist/esm, and dist/es2015, and dist/es2015/esm (another reason for this PR from my side). Rollup greatly simplifies this configuration (and I'll be honest there is likely an even more optimal way to achieve this than I did here)

  4. There is no additional es2015 build. The benefits here for bundle size primarily.

  5. This configuration takes advantage of the forward compatible extensions (.cjs and .mjs) for node environments.

  6. exports is also forward compatible (and compatible with Node all the way back to 12. Its just ignored otherwise). There's future proofing there for how bundlers / node resolves packages

I wish I could find the article I read months ago from early 2022, but I recall flat file builds are suppose to be easier to do dead code elimination as well for bundlers. I can't seem to find it though.

@ScottAwesome
Copy link
Author

ScottAwesome commented Sep 6, 2022

I could just swap this for microbundle too.

Looks like the main blitz.js repo uses unbuild for everything. I'd be happy to align with that if that works better here, but I'm unsure if you can get dual exports out of that model (ES2015 and ES5)

@Skn0tt
Copy link
Collaborator

Skn0tt commented Oct 10, 2022

Thank you for outlining the pros and cons! Personally, I'm a fan of keeping things simple. I don't see a convincing benefit from changing the bundling mechanism in this list, and would rather wait for tsc to add support for some of the listed features. Having said that, I do think there's value in keeping bundling similar across blitz repos - @beerose what are your thoughts on this?

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.

2 participants