[go: up one dir, main page]

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: improve support for code splitting, ditch umd, sourcemaps #1102

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cyyynthia
Copy link
Contributor
@cyyynthia cyyynthia commented Jul 6, 2024

This PR improves code splitting support by keeping each component namespace within its own chunk, allowing Rollup to move parts of the library in lazy-loaded chunks instead of entirely hoisted in the entry file. While this is the main motivation of this PR, I also tried a few things with the config that I think are worth sharing:

  • The UMD bundle has been removed in favor of a CJS build; I don't think the UMD config was correctly configured for it to function as expected. (Mentioned in the Discord server)
  • TypeScript declaration files are now passed through Rollup to be a single file; this significantly reduces the package size (-1.6M) at the expense of slightly less pretty declaration files.
  • Target has been set to esnext, which pretty much disables processing of recent ES features. That's quite an opinionated change; the assumption I'm making is that the package will likely be re-bundled so not processing is the best for this scenario. This may not be the case for CJS builds, though.
  • This PR also attempts to give back a look at chore: enable sourcemaps for radix-vue package #469. Enabling source-maps adds ~1.4M to the bundle per format, for a combined 2.8M increase in package size (from 1.8M to 4.6M). That's unfortunately more than double, especially knowing that most of it is duplicate sourcesContent in esm and cjs maps.

The PR is a draft at this time with my experiments; as they might cause breaking and what not I'm not sure if it fits for v1 or if it may directly go to v2. Most points are also up for discussion and will need an opinion from the maintainers in favor or against as they affect package quality, target and size.

For the reference: current package size is 3.2M. Measures taken by running pnpm build-only in packages/radix-vue and running du -sh dist.

@sadeghbarati
Copy link
Collaborator
sadeghbarati commented Jul 8, 2024

Merging all .d.ts files with the rollupTypes is a good decision, it also helps Volar to resolve types faster


Dropping umd is LGTM cause there are so many ways to use modules in the browsers without the bundlers, for example https://play.vuejs.org/ is using <script type="importmap"> and <script type="module"> in the browser

https://caniuse.com/es6-module


Dropping cjs build is not good at the moment cause we don't know what the user bundler is, Vite, Webpack, Snowpack, Rspack, Farm, Rolldown, Rollup, Turbopack, esbuild, ... 😶‍🌫️

https://github.com/wooorm/npm-esm-vs-cjs

@cyyynthia
Copy link
Contributor Author

I'm unsure the problem is what bundler they are using, but rather if they have a bundler to begin with. ESM have been around for a decade, and I have yet to hear about a bundler that doesn't support ESM. I think it's more useful when targeting backend applications which are generally the ones running in CJS environments.

Regardless, after looking a bit at some big libraries it seems Vue & its first-party libs (Pinia, Router, ...) all have CJS and UMD builds available. Same story for some other popular component libraries. So I think it's a good idea to keep support for at least CJS (UMD being less and less useful as you mentioned)

However I'm wondering if the CJS build shouldn't be built separately, and use a different build configuration:

  • Single-file output (as in, just the entrypoints)
  • No sourcemaps

This would most likely keep the package in reasonable sizes, provide sourcemaps for use with bundlers, and for server-side or very legacy usages CJS would be available. I can try giving a look at going towards this direction.

@cyyynthia
Copy link
Contributor Author

I've implemented the proposed changes to cjs bundling; package is now sitting at 3.1M, about the same as currently, with esm+maps and single-file cjs w/o maps. 🎉

I've also added a new chunk to isolate ToastProvider and TooltipProvider separately from their namespace; these two are very likely to be used at root-level, while toasts themselves or tooltips may only be used in lazy-loaded chunk(s).

@zernonia
Copy link
Member

That's an amazing job @cyyynthia ! Loving the approach you had and the results!

Perhaps we need to somehow improve the generated types. It has 1 whole file and causing alot of duplication. This also affect the DX to inspect the component types and find related props/emits/slots/expose and their docs.

image

image

@sadeghbarati
Copy link
Collaborator

To prevent types duplication, vueCompilerOptions.skipTemplateCodegen: true might help?

@cyyynthia
Copy link
Contributor Author
cyyynthia commented Jul 14, 2024

The only way I found to get rid of these is a super hacky find & replace, and running eslint on it to get it a bit more readable, not sure if it's a great setup though 🤔

  • declare type __VLS_([a-z]+)_\d+((.|\n)*?)};\n\n => empty
  • __VLS_([a-z]+)_\d+ => __VLS_$1
  • Run pnpm exec eslint --no-ignore --rule 'ts/ban-types: off' --rule 'import/no-duplicates: error' --rule 'ts/consistent-type-imports: off' --fix dist/index.d.ts dist/date.d.ts

I haven't checked if it improves the DX of the single-file dts so I don't know if it'll make a difference (unless Vue relies on types having precisely the right name, idk); however I do know that a single .d.ts is faster to resolve, and is 1.8M than the current types output; it'd be really nice to get it to work nicely 🤔

@cyyynthia
Copy link
Contributor Author

👀 fb372d6

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.

3 participants