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
feat: add an ESM publish step #485
feat: add an ESM publish step #485
Conversation
}) | ||
|
||
const plugins = [ | ||
builtins(), |
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.
something uses util
so builtins were required.
const plugins = [ | ||
builtins(), | ||
nodeResolve(), | ||
commonjs({ |
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.
Required to work with deep-equal
why is this needed? bundlers that are working properly don't need much "support" to bundle node modules, they just bundle them. CJS can be imported in ESM, and bundlers should be able to handle that. |
You seem to incorrectly assume that everybody uses a bundler for development |
I'm assuming that anyone using modules from npm is using a bundler. If you're not using a bundler, you're cutting yourself off from the largest repository of shared code in human history. |
Your assumption is again wrong. We use buildless development at work for a very large enterprise app. During development there is no bundling involved, yet we still make use of the largest repository of shared code in human history. |
that's npm, the majority of which objectively requires a build process, so i'm not sure what you mean. minification, or generating import maps, are both a build process. |
We use npm dependencies in our app. We don't bundle during development. |
@ljharb In case you're not aware, the folks at https://modern-web.dev/ have been doing a lot of solid work to promote buildless dev workflows. Note that their tooling expects ESM out of the box. Any npm dependency that provides an ESM native version of its code just works. Import maps aren't always required and minification isn't necessary or useful for all apps and environments. You now have at least two people explaining to you how your assumptions are wrong. Three if you count the author of this PR. Four if you count the person who originally wrote the linked issue. |
@jimsimon which assumptions? (edit: ah, when i said “I’m assuming” above :-) perhaps i phrased it badly, what i meant was, “i choose to assume” not that i believe that’s actually true) I realize nonzero npm deps can be used without a bundler - i said the majority above. I also realize that if you remove enough of the typical constraints of web dev (for example, if you’re not targeting the open web ie for an internal app, or if you don’t care about practical cross-browser support because you can get away with requiring specific browsers and versions) that you certainly can have a no-build-process app. I’m also aware that some module tools choose to artificially restrict their input, which can, when using packages that are thus restricted, create either inconvenience, or cause complexity and sources of bugs to cascade through the ecosystem. Adding additional build processes in every package you run into this with, and additional files in the shipped package, is a high cost for use cases that are not likely to ever be practical for everyone to benefit from. |
With respect, can you just stop gatekeeping on a repo you dont maintain? Having a package available as ESM is valuable, end of story. |
@thepassle how can it be gatekeeping if I’m not operating the gate? Adding complexity to this package affects my packages that use it, and all the millions of downstream consumers, so I’m not going to stop pushing back on harmful changes to this package. |
Add some package exports, make it a breaking change, and you should have no issues 👍 |
@thepassle breaking changes are extremely costly, since very few maintainers backport security fixes to previous majors. The issue is "the latest version of a package i depend on will be more bug-prone and heavyweight". |
You’re reaching man |
I'm sorry you think so. I maintain a lot of packages that are used by a lot of people, and I'm speaking from extensive experience. It's fine that that experience doesn't match yours. |
Omg :’) |
@ljharb, would you mind clarifying "if you’re not targeting the open web ie for an internal app, or if you don’t care about practical cross-browser support because you can get away with requiring specific browsers and versions"? Support for which specific feature in which browser would you be missing? (In case this is about import maps in Safari: 16.4 should support them. And as Jim mentions above: You don't always need them. And they can be polyfilled). UPDATE: As a matter of fact, I can already use Konnor's fork in current Safari without any build steps or polyfills: <script type="module">
import { roles } from "./node_modules/aria-query/lib/esm/index.js";
console.log(roles.get("alert"));
</script> |
There are many tools that can easily work in browsers except and only except for the fact that they are CJS-only published. Therefore, we lock the web development ecosystem into having to use bundlers simply for format. This is unhealthy for the ecosystem. As browsers have matured and still are, getting rid of bloated development environments full of bundler magic has become not just possible, but easy! and it goes a long way of getting rid of JS Tooling fatigue and developer experience slowdowns. We're not asking to change the main format of this library, we're merely asking to publish and ESM variant next to it so tooling bloat can be reduced for web devs, so that we may hopefully, one day, find inner peace. It is silly to claim that this introduces "risk" for CJS consumers of this package. |
Pull Request Test Coverage Report for Build 4265739603
💛 - Coveralls |
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.
If you end up doing this anyways, then these are the "exports" changes you'd need to properly support native ESM in the most backwards-compatible way.
Note that adding "exports" is a breaking change (and arguably adding "module" could be as well, but it's less likely since it's not a standard field)
@@ -3,11 +3,12 @@ | |||
"version": "5.1.3", | |||
"description": "Programmatic access to the ARIA specification", | |||
"main": "lib/index.js", | |||
"module": "lib/esm/index.js", |
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.
"module": "lib/esm/index.js", | |
"module": "lib/esm/index.js", | |
"exports": { | |
".": [ | |
{ | |
"import": ""lib/esm/index.js", | |
"default": "lib/index.js" | |
}, | |
"lib/index.js" | |
], | |
"./package.json": "./package.json" | |
}, |
Why do we need to add exports for ES modules with this PR when we haven't supported that before?
Yep, wanted to ship this with 6.0 anyway. |
@eps1lon the whole argument for adding an ESM build seems to be that it's easier to consume, but surely that includes node itself, which requires the "exports" field for ergonomic consumption. Without it, the only way to import this module's native ESM would be to import Also, adding "exports" means that everything not in "exports" is not accessible at all, so it greatly reduces your package's observable API. Totally separate from my opinions about shipping native ESM, it is always a very good idea for packages to use the "exports" field (in concert with the "main" field). |
I appreciate the debate here (@ljharb I always value your opinion). I need time to review the arguments for and against. |
@eps1lon the “easy” way around this is to have Rollup produce only 1 output file with index.js I didn’t do this because the previous babel build process output the full directory structure. I selfishly only used module / main since that’s what @testing-library/dom was using, but the technical correct implementation is to map all files to exportmaps, assuming you want to expose the full directory structure and not just use a single entrypoint. |
Note that in package exports you can add |
It's been a while and I realize I closed this and never added an explanation. Closed because I really don't have the time or energy to argue with someone over something I was doing to try to help and noticed there was another issue around shipping an ESM compatible package. If someone else wants to take up this PR go for it, but I have 0 desire. I forked this package and moved on. |
Understandable @KonnorRogers this whole discussion is exhausting, shame people are pushing back against ESM and keeping bad practices alive (requiring bundlers to consume browser packages because of format). Hopefully this still ends up landing on v6 at some point. could you link your fork so I can use it instead of this package? Then I too can get rid of my overcomplicated build setup in my project. |
https://www.npmjs.com/package/aria-query-esm https://github.com/KonnorRogers/aria-query/tree/konnorrogers/publish-modern-build @jessebeach Apologies if I missed anything in the Apache 2.0 license, let me know if there's anything else I need to do to make it compliant. I added a notice of derivative work. |
I know nearly zero about licensing =D Fork and be happy =D But seriously, are you blocked or do you feel like you can move forward with what you want to do here? |
This creates ESM bundles for supporting bundlers.
All ESM bundles are accessible under
./lib/esm/*
I couldnt get the test suite to run, some issue with flow spawning. Probably related to M1. Happy to answer any questions!
Of course when I linked this locally, I just ran into another issue with a different package (
lz-string
) but at leastaria-query
isnt the culprit! I shouldve just had Rollup fix the dependencies for me like here: modernweb-dev/web#1877But alas, here we are! Hope it helps!
Fixes #48