-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
base: main
Are you sure you want to change the base?
Support browserslist + initial size limit and es-check scripts #1041
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
For example of browserslist you can try out different scenarios here: https://browserslist.dev/?q=ZGVmYXVsdHMsIGNocm9tZSA%2BPSA5NixmaXJlZm94ID49IDkwLGVkZ2UgPj0gOTEsc2FmYXJpID49IDEyLGlvcyA%2BPSAxMixvcGVyYSA%2BPSA5MA%3D%3D |
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. |
Regarding browserslist
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
PropsalIn my last commit, I've added this suggested baseline:
It should cover 94% of browsers at minimum Size-limit: the same And I think it's a valid choice (made by esbuild) Reasons to set itI 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. |
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. Why it's usefulEstimating 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... |
@fabian-hiller if you feel convinced and wants to proceed... my steps are
Then
|
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.
|
Not sure I understand the relation between browserlists and tree shaking and embed (you mean install ?)
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
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. |
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 ? |
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?
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. |
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. |
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 :) |
Forgot to say. I’m very fan of the work you did on valibot and standard schema. It’s very welcome in the ecosystem |
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. |
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
pnpm check.size
with a temporary size-limit.tspnpm check.dist
to ensure bundled code is es2022 compliantNext 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 bundled10.34 kB
For example if we want to support more browsers
es2022,chrome96,edge90,firefox90,ios14,opera84,safari14
The bundle would be2. 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.
Open to discuss...