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

[WIP] chore: use NodeNext module resolution and use tsc for builds #250

Closed
wants to merge 3 commits into from
Closed

Conversation

fubhy
Copy link
Collaborator

@fubhy fubhy commented Mar 27, 2023

Fixes #244

NOTE: For now, this is just meant to help illustrate what I'd suggest for the build setup and type config. Feel free to reject this if you'd rather stick with tsup. There are some other tsconfig options (see tsconfig.base.json) that I'd suggest to enable too if you decide to roll with this (some of them currently fail if enabled, would require a bit of refactoring).

ALSO: This is still a work in progress of course. I've also removed the import from package.json to load the version to keep and force/restrict the build root to src. This means we'd now need a small script or prebuild command to copy & replace the version in src/errors/version.ts.

Happy to discuss and continue this further if you want to explore this more.

@changeset-bot
Copy link

changeset-bot bot commented Mar 27, 2023

⚠️ No Changeset found

Latest commit: f35da83

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Mar 27, 2023

@fubhy is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 27, 2023

To also show what I meant about code navigation and source map publish I added another commit that publishes the source code and source maps & declaration maps too. This allows library consumers to navigate to the original source code from the type declarations (d.ts) files. I find that immensely helpful when working with libraries like these.

@jxom
Copy link
Member

jxom commented Mar 29, 2023

Thanks for the PR! Love the super pragmatic approach, and agree that developing in an environment similar to the consumer's environment would be nice. Haven't had much time yet to dig closer into this, but from a high-level, I have a few questions (I am not too familiar with tsc as a bundler):

  • Could we utilise TypeScript 5's moduleResolution: "bundler" with tsup to help resolve bug: type inferrence not working fully with NodeNext moduleResolution #244?
  • How will bundling with tsc affect tree-shaking/is tree-shaking well supported?
  • How will bundling with tsc affect bundle size, does it have some smarts around optimization here?
  • How does tsc stack up with tsup in terms of bundle time? Is it still bearable?
  • Is it at all possible to use it without adding .js extensions to everything?

Agree that declaration maps will definitely be a nice addition, but I don't think tsup supports them yet 😅. One thing we could do if we still want to roll with tsup is run a step after the bundling process to run tsc --emitDeclarationOnly or something.

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 30, 2023

@jxom Excellent questions! And I love that you are concerned about tree-shakability! A lot of libraries simply don't care or make subtle mistakes that are hard to fix later without introducing breaking changes (reorganizing exports, removing side effects, etc.). A lot of documentation out there is super outdated, confusing or outright wrong though so I can't blame anyone. To your questions:

How will bundling with tsc affect tree-shaking/is tree-shaking well supported?

Tree shaking is the job of the bundler used by the consuming application. The only thing we have to do here is:

  1. Compile and publish (correctly) an ESM build

Yep, tsc is perfectly capable of doing that.

  1. Avoid side-effects like the plague (side-effects in this context refers to any root-scoped code that runs on import of a module).

Viem is currently free of side-effects. At least I didn't see anything yet that would suggest otherwise. Yai!

  1. Declare your package as side-effects free (or explicitly isolate and specify those entrypoints that have side-effects).

You are already doing this. Yai! https://github.com/wagmi-dev/viem/blob/main/package.json#L122

  1. Don't (!) bundle your library code

This might be obvious but I'm pointing this out because in your questions you referred to "bundling" several times and I wasn't sure whether you used it colloquially or not. So apologies if I'm stating the obvious here:

You do not want to bundle your library code (webpack, parcel, etc.)! We just want to transpile / compile it from typescript to javascript (CJS or ESM). Whether you do that with tsc or using a different tool doesn't matter as long as said tool doesn't garble your modules in a way that breaks tree-shakability: It's generally advisable to do as little as possible at this stage. Using a bundler here can easily break all your efforts to make your library tree-shakable though. You can't go wrong with tsc here (and it's likely your best option), because it retains the full granularity of your modules giving the maximum flexibility to the downstream application bundler.

There are exceptions of course. For instance, if you wanted to provide a plug & play UMD bundle, which can be handy in some situations.

I would suggest to also test for that within the examples packages: set up vite, turbopack, esbuild, swc, etc. examples and see how well they deal with eliminating dead code from their imports. Not sure how to best go about that... We could dump the output of du -h on the generated build output into a snapshot file for instance? Maybe there are github actions to faciliate that.

TL;DR: bundling is the job of the consuming application. Imho, your best option for publishing both your js modules & types (and maps) is tsc.

I went around shopping for solid blog posts that further discuss this whilst sticking to the point. I think this one does a good job at that without going too much into the weeds: https://blog.theodo.com/2021/04/library-tree-shaking/

How will bundling with tsc affect bundle size, does it have some smarts around optimization here?

I think I answered that in the previous answer: As long as we cleverly set up our barrel exports (btw. I would advise to not use imports from barrel files within the library itself, it has bitten me before), and don't break tree shakability by (incorrectly) bundling our ESM exports, we should get the ideal result for downstream applications to optimize their bundle sizes.

How does tsc stack up with tsup in terms of bundle time? Is it still bearable?

There's no denying that esbuild is incredibly powerful & fast. As a result, tsc is orders of magnitude slower than tsup. That's a bummer of course. In relative terms, the difference in build speed will be significant. But since, in this case, we are talking about a few miliseconds vs. a few seconds, the absolute difference is still small and shouldn't matter much since you can avoid it for local development and it would only affect publishing & ci runs (a few seconds).

Could we utilise TypeScript 5's moduleResolution: "bundler" with tsup to help resolve #244?

Is it at all possible to use it without adding .js extensions to everything?

Requiring the .js extension is a result of me changing moduleResolution to NodeNext. That change is most likely not necessary to fix the type inferrence problem. I'll admit that I got a bit carried away there in trying to bump your configuration to the bleeding edge ;-) ... I initially only introduced this change to your tsconfig.json in an attempt to surfice the type inferrence problem self-sufficiently by building your examples with this configuration too. Either way, I think it would make sense to leverage your examples not only as documentation and playground for development but also to shield against regressions. I'd consider building all examples (with different tsconfig.json setups) as part of CI.

I agree that it looks a bit odd at first after years and years of not using any file extension in your imports in js & ts land ... Takes a bit of time to get used to it. But IDEs already widely support it and I haven't had any issues with it so far. I haven't experimented with the upcoming bundler option yet but just reading the announcement I'd still suggest to go with NodeNext and get used to it :-). It has it's benefits indeed! But to be sure, I'll give bundler a test spin too ...

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 30, 2023

Btw. for full transparency regarding my stance on publishing source maps & declaration maps... It is controversial and it's matter of taste. I'm personally leaning pretty strongly towards publishing source code & source maps & declaration maps together with the package.

You've got two options:

  1. Publish source code, source maps and declaration maps all together
  2. Do not publish source code, source maps or declaration maps at all

There's not really a middleground because publishing source maps without source code basically leaves you with references to files (in your sourcemaps) that aren't included in your package. That has the potential to trip up tooling that tries to parse your source maps (e.g. for stack traces, etc.) and can potentially break things.

So here are the benefits & downsides of publishing all of this with the package (my recommendation):

Benefits

  • Added utility in IDEs, e.g:
  • Ability to not only jump to type declarations but also to the underlying source code
  • Added utility for error handling & tracing services, e.g:
  • Ability to create awesome error stack traces also for library-scoped errors
  • Conveniently debug library related problems by browsing the included source code instead of having to clone the corresponding repository (at the right commit hash corresponding to the used package version)

Downsides

  • Increased package size

For a plain typescript library, the increase in package size isn't significant enough, imho, to really argue against doing this. If your src directory had any large binary (or other) files in it (e.g. images, etc.), I'd simply exclude those from being published.

Related discussion: googleapis/google-cloud-node#2867

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 30, 2023

Btw. is there a discord server that you guys hang out in?

@jxom
Copy link
Member

jxom commented Mar 30, 2023

Sorry, excuse my language of using the term “bundling”! I definitely meant “compiling” (I was writing that response half awake 😜).

Also definitely want to move away from the index/barrel imports from within viem itself as you said — we just recently discovered this has some nuances with tree-shaking (for example, importing a simple util pulls in 10kB gzipped or something when it should be much lower depending on the util).

Open to adapting to the .js extension on imports as well!

Definitely down to publish declaration maps as well, no brainer in terms of DX of digging into viem source code as a consumer.

@fubhy
Copy link
Collaborator Author

fubhy commented Mar 30, 2023

Superceded by #265 . Will come back to NodeNext in a separate, dedicated PR to reduce the noise from all the import changes.

@fubhy fubhy closed this Mar 30, 2023
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.

bug: type inferrence not working fully with NodeNext moduleResolution
2 participants