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

Support browserslist + initial size limit and es-check scripts #1041

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

belgattitude
Copy link

@belgattitude belgattitude commented Jan 30, 2025

Following that discussion: #1018 (comment), I'd like to take a look a bundle performance.

Before doing so I'd like to get a baseline ensured. Here's the changes I propose

Features

  • Add pnpm check.size with a temporary size-limit.ts
  • Add pnpm check.dist to ensure bundled code is es2022 compliant
  • Add initial browserslist configuration to ensure bundled code is browser compliant

Next steps

1. Browserslist

I feel we should decide what browserslist compat we want. Locally I've tested impact on the bundle size:

By default es2022,chrome109,edge131,firefox128,ios15.6,opera113,safari18.1 will produce a bundled 10.34 kB

✔ Adding to empty webpack project
  
  Size limit: 10.4 kB
  Size:       10.34 kB with all dependencies, minified and brotlied

For example if we want to support more browsers es2022,chrome96,edge90,firefox90,ios14,opera84,safari14The bundle would be

✔ Adding to empty webpack project
  
  Package size limit has exceeded by 181 B
  Size limit: 10.4 kB
  Size:       10.58 kB with all dependencies, minified and brotlied

Note that transpiling for older browsers might not only affect size but also speed

2. Add more scenarios to size-limit.ts

Ensure we have sufficient imports covered to ease eventual optimizations and prevent unseen regressions in size.

3. Add check to CI

4. Evaluate import { v } from valibot

From this P/R once we have a baseline, I'm open to tune the tsup config to emit separate files and try out mulitple scenarios.

Note that bundling many files will probably heps tree-shaking but node_modules install size might be bigger

Open to discuss...

Copy link

vercel bot commented Jan 30, 2025

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

Name Status Preview Comments Updated (UTC)
valibot ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 11:56am

@belgattitude
Copy link
Author

@fabian-hiller
Copy link
Owner

Hey, thanks for creating this PR and for your further research for possible improvements in this regard.

How compatible is Valibot's current bundle? I wonder if we even need Browserslist. I try to keep things as simple as possible, but if there is a value, I will consider it.

What does size-limit do? I ask because for Valibot the total bundle size isn't really important due to our modular architecture. Adding new features doesn't grow the bundle for users who don't use them. But maybe I have misunderstood size-limit.

@belgattitude
Copy link
Author

belgattitude commented Jan 30, 2025

@fabian-hiller

Regarding browserslist

I wonder if we even need Browserslist. I try to keep things as simple as possible, but if there is a value, I will consider it.

If simplicity is a goal browserslist will do. It's mature and supported by all transpiler/minifiers/bundlers (swc, esbuild/tsup, webpack, babel ...). The browserslist config will inform them about what features shouldn't be transpiled.

The idea behind is simple. Code with latest ecmascript features, let the transpiler decide how to replace syntaxes that aren't supported by target. For the maintainers and the reviewers it's much easier.

Contrived example of a PR including a js syntax that would break ios < 14: nullish colascing operator. Using this feature would potentially create a ticket leading to a fix.

As of today most browsers, including node, deno and bun supports most of es2024 (except decorators). Browserlist helps to inform the transpiler/bundler about specific features that should be transpiled.

What's at stake ? A javascript syntax error can really break an entire website. So it's good to get it under control with proper tooling rather than conventions or reviews. We don't know what es202x will bring in the future.

Another aspect and it's quite important: communicate which browsers valibot officially support. That eventually helps devs to choose to transpile the library (ie: https://nextjs.org/docs/app/api-reference/config/next-config-js/transpilePackages) based on their compatibility requirements (some projects should be usable across enterprise systems that tend to work with really outdated browsers: firefox esr, edge 19..).

What browserslist doesn't do

  • The actual transpilation. It just inform the transpiler,minifier...
  • Polyfilling. ie: Promise.try (some bundlers might use it to polyfill)
  • AFAIK it doesn't transpile down regexps

Propsal

In my last commit, I've added this suggested baseline:

defaults
chrome >= 96
firefox >= 105
edge >= 113
safari >= 15
ios >= 15
opera >= 103
not dead

It should cover 94% of browsers at minimum

Size-limit: the same
Dist code: only one difference:

image

And I think it's a valid choice (made by esbuild)

Reasons to set it

I feel it's always a good idea to have one. For example if valibot suddently receives bug reports about a particular browser version (safari ios ...), it's easier to tackle it straight from the tooling than in the code. We can even imagine having 2 dist files. One modern, one compat. But nowadays it's less an issue. And also we don't now the future.

@belgattitude
Copy link
Author

belgattitude commented Jan 30, 2025

@fabian-hiller

Why size-limit ?

You can check my last commit. You'll see a more interesting example

import type { SizeLimitConfig } from 'size-limit';

module.exports = [
  {
    name: `import * as v from 'valibot' (ESM)`,
    path: ['dist/index.js'],
    import: '*',
    limit: '10.35KB',
  },
  // Core
  {
    name: `import { parser } from 'valibot' (ESM)`,
    path: ['dist/index.js'],
    import: '{ parser }',
    limit: '335KB',
  },
  // Functions
  {
    name: `import { pipe } from 'valibot' (ESM)`,
    path: ['dist/index.js'],
    import: '{ pipe }',
    limit: '370B',
  },
  // Schemas
  {
    name: `import { array } from 'valibot' (ESM)`,
    path: ['dist/index.js'],
    import: '{ array }',
    limit: '815B',
  },
] satisfies SizeLimitConfig;

size-limit can use webpack or esbuild to really estimate the cost of a specific import. as ebpack is probably the most used bundler, I kept the configuration with webpack.

image

Why it's useful

Estimating the impact of a refactoring or a build change is not always easy. A simple change can make the bundle grow unoticed.

Is it useful now ?

Probably yes if we want to experiment with different bundling techniques. On the long run of course the size limit could be part of ci or a reporting action...

@belgattitude
Copy link
Author

belgattitude commented Jan 30, 2025

@fabian-hiller if you feel convinced and wants to proceed... my steps are

  • Add arethetypeswrong check
  • Add packageManager field

Then

  • Try to bundle in separate files (that would make most of // @__NO_SIDE_EFFECT style annotations obsolete) but it's not totally ideal in m experience. Some drawbacks
  • Try to allow import { v } from 'valibot'

@belgattitude belgattitude changed the title [WIP] Support browserslist + initial size limit and es-check scripts Support browserslist + initial size limit and es-check scripts Jan 30, 2025
@fabian-hiller
Copy link
Owner

My understanding is that most Valibot users don't embed the library code as is into their application. Instead, they run a build step that bundles their application to apply things like stree shaking. So I am not sure if Valibot should implement browserslist. It feels like developers who are using Valibot and shipping it as part of a website to a browser should instead add it to their build step.

size-limit is generally a good idea, especially for a library like Valibot that prioritizes small bundles. I don't think we should add it right now, but we should evaluate it after our stable v1 release to ensure a small bundle size for major changes in the long run. It might make sense to check common scenarios, such as validating a login schema, for bundle size changes.

@belgattitude
Copy link
Author

belgattitude commented Jan 30, 2025

most Valibot users don't embed the library code as is into their application. Instead, they run a build step that bundles their application to apply things like stree shaking. So I am not sure if Valibot should implement browserslist. It feels like developers who are using Valibot and shipping it as part of a website to a browser should instead add it to their build step.

Not sure I understand the relation between browserlists and tree shaking and embed (you mean install ?)

they run a build step that bundles their application to apply things like stree shaking... website to a browser should instead add it to their build step.

Totally the build step will use a bundler (either vite, webpack, ...), but, correct me if I'm wrong... they don't transpile dependencies imported from node modules (deno is different in that matter).

So imagine, you install reack-hook-form, react-query, valibot... they are installed in node_modules. Whatever you build your application with (nextjs, remix, svelte,.... - vite/webpack/...), the dependencies will just be inlined into the build outputs (dce then chunks then minify...).

Only the application source code is transpiled (ie babel, swc), not the node_modules/deps. (at least not by default). If you're unsure maybe take a look to some projects that are generally bundled for browser usage:

ie: react-query : https://github.com/TanStack/query/blob/main/scripts/getTsupConfig.js ... and probably all frontend libs that gained some popularity in the last two years (cause at that time things like coalescing operator wasn't supported like now, so it was easy to use them and create issues)

Other wise no big deal. Valibot code seems to be very compatible in it's current form

size-limit is generally a good idea, especially for a library like Valibot that prioritizes small bundles. I don't think we should add it right now, but we should evaluate it after our stable v1 release to ensure a small bundle size for major changes in the long run. It might make sense to check common scenarios, such as validating a login schema, for bundle size changes.

Yes good idea... you'll find many examples in the wild. The fact is to get measurements at some point, less asumptions.

Good luck for the v1.

@belgattitude
Copy link
Author

By the way I started the experiment to find ideas about this question : #1018 (comment) and especially the import { v } from valibot...

Does it make sense to go on ?

@fabian-hiller
Copy link
Owner

I thought that bundlers also apply the target setting to its dependencies, but I think you are right and bundlers only bundle, tree-shake and minify the code.

What advantage do you see in using a browser list instead of just setting "ES2020" as the target?

By the way I started the experiment to find ideas about this question ... and especially the import { v } from valibot...

I'm not sure if we should support two ways to import the library via the "v" letter, as I'm pretty sure it will confuse a lot of people and cause questions about which is better. I see the DX advantage, but maybe we should taggle that with a VS code extension instead.

@belgattitude
Copy link
Author

What advantage do you see in using a browser list instead of just setting "ES2020" as the target?

Runtimes generally implements the ecma standards progressively. By runtime I mean, node, deno, edge, cloudflare, v8, webkit, hermes in react native… so targeting an ecma target is generally not enough to cover compatibility in the wild. I’d say nowadays it’s less an issue as ecma haven’t introduced new syntax changes since long. Ie null coalescing operator is baseline nowadays.

That said having browserslist ready is a good thing imho. The thing in my experience is that there’s always a moment where you’ll get bug reports or questions.

It’s also a no op on the build system. It virtually does nothing except informing the transpiler about features available. In a more nuanced form than saying ie es2024.

@belgattitude
Copy link
Author

I'm not sure if we should support two ways to import the library via the "v" letter, as I'm pretty sure it will confuse a lot of people and cause questions about which is better.

You’re totally right. I don’t consider it important myself. But I’m sure the ecosystem will converge to named exports at some point. I can clarify in another pr if you like. Might fix issues in others environments too, ie expo, rn… where the support for import * as isn’t really a priority.

On my phone so it’s a rough explanation :)

@belgattitude
Copy link
Author

Forgot to say. I’m very fan of the work you did on valibot and standard schema. It’s very welcome in the ecosystem

@fabian-hiller
Copy link
Owner

Thank you for your words and thank you for helping to improve Valibot. I have almost no time at the moment and have to focus on the most important parts to ship our v1 release in the next weeks. Let's discuss the PR again after that.

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