-
-
Notifications
You must be signed in to change notification settings - Fork 150
Drop separation between Node.js and browser builds #649
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
Conversation
1961f2f to
386ab7d
Compare
FYI, according to https://packagephobia.com/result?p=yaml, it's currently at 667 KB, not 1.2MB. The 257 published file count seems quite large, I hope that will be reduced as well. |
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| node-version: [18, 20, latest] | ||
| node-version: [20.19.0, 22.12.0, 24.0.0, latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend to continue using major version specifiers like [20, 22, 24, latest] because that's more representative to what most users will be running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 20.19.0, 22.12.0, 24.0.0 are meant to catch violations of the minimum supported versions, which are specifically these.
In my experience, latest is the best target for catching issues due to Node.js changes; I've not seen any issues caused by fixes backported to older major versions, which is what 20, 22, 24 would catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah those stable branches are indeed generally very stable, so seems like a fine approach.
| "node": ">= 14.6" | ||
| "node": "^20.19 || ^22.12 || >=24" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding sideEffects: false to package.json to enable bundlers to tree-shake the module. For example if only parse is used, no need to bundle stringify. This flag is respected by many bundlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With bundling, the effect of sideEffects would be pretty much nonexistent. Considering your specific example, YAML parsing and serialization is actually interlinked, so it's not possible (at least with this library's current choices) to drop stringify when using only parse, as that's required by mappings with complex keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Maybe in the future you find ways to (mostly) decouple them. It would be a nice benefit if around half the library could be compiled away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parser needs the stringifier to deal with keys like this:
import { parse } from 'yaml'
parse('[foo]: bar'){ '[ foo ]': 'bar' }Using JSON for that doesn't work for all cases, because it can't deal with self-referential values (which are possible in YAML with anchors and aliases).
The stringifier needs the parser to figure out when content needs to be quoted:
import { stringify } from 'yaml'
stringify(['Tru', 'True'])- Tru
- "True"So decoupling the two completely isn't possible, but maybe some work here could be done by identifying the parts that are strictly needed, and making sure to rely on only those. I welcome others' efforts towards this, but it's not a personal focus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if the functions have shared dependencies, that's fine and partial tree-shaking would still be possible. I would recommend adding sideEffects: false, which basically tells the bundler "my exports are pure and any unreachable code can be removed".
Many popular packages set this flag already and I've never heard of anyone having problems with it. If your code does not do dirty things like mutating global state, it should be safe to use this flag and give bundlers this additional hint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, the intent is to have yaml itself be bundled for publication, which means that its consumers will be effectively importing a single module per endpoint (possibly with some common chunks). As the sideEffects: false toggle only effects module loading, this means that it will have no effect here.
I'm happy to work towards decreasing the tree-shaken size of the library further, but that should be done in ways that are actually effective.
I'm not sure how they're measuring file size, but on my MacBook I see this: |
|
Right, I see similar, 792kB in |
|
Merging in order to work on next steps separately, and not pile them on here. |
The dual browser + Node.js build using Rollup is dropped, and replaced by a single Rolldown build producing ES modules.
The most significant change for users is that the default export is dropped, and so the following change may be required to user code:
The minimum supported Node.js and TypeScript versions are updated to what's needed for
require(esm)support, i.e. Node.js 20.19 and TS 5.9; similarly, the library is now tested in Chrome 93 and Firefox 94, though no specific versions of browsers are explicitly supported without transpilation.Getting the
test:disttask to work with Jest was too much hassle, so this also includes a switch from it to Vitest, which effectively "just works". It's also configurable to run (most of) the whole test suite in browsers, so that's now done rather than the minimal smoke test that was used before.I was not able to figure out how to configure Vitest with the WebdriverIO provider to get it to work with Browserstack, and so switched to Playwright; getting that to then also work from GitHub Actions was yet another hassle.
All of the above also allows dropping the tests from the playground repo, so it doesn't need to be handled as a git submodule any more, and it's only the playground now.
After these changes, the deployed on-disk size of the library is reduced from around 1.2 MB to 0.3 MB, and the compressed size reduces from 107 kB to 71 kB.