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

feat: add an ESM publish step #485

Conversation

KonnorRogers
Copy link

@KonnorRogers KonnorRogers commented Feb 20, 2023

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 least aria-query isnt the culprit! I shouldve just had Rollup fix the dependencies for me like here: modernweb-dev/web#1877

But alas, here we are! Hope it helps!

Fixes #48

})

const plugins = [
builtins(),
Copy link
Author

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({
Copy link
Author

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

@ljharb
Copy link

ljharb commented Feb 20, 2023

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.

@thepassle
Copy link

You seem to incorrectly assume that everybody uses a bundler for development

@ljharb
Copy link

ljharb commented Feb 20, 2023

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.

@thepassle
Copy link

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.

@ljharb
Copy link

ljharb commented Feb 20, 2023

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.

@thepassle
Copy link

We use npm dependencies in our app. We don't bundle during development.

@jimsimon
Copy link

@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.

@ljharb
Copy link

ljharb commented Feb 20, 2023

@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.

@thepassle
Copy link

With respect, can you just stop gatekeeping on a repo you dont maintain? Having a package available as ESM is valuable, end of story.

@ljharb
Copy link

ljharb commented Feb 20, 2023

@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.

@thepassle
Copy link

Add some package exports, make it a breaking change, and you should have no issues 👍

@ljharb
Copy link

ljharb commented Feb 20, 2023

@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".

@thepassle
Copy link

You’re reaching man

@ljharb
Copy link

ljharb commented Feb 20, 2023

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.

@thepassle
Copy link

Omg :’)

@backflip
Copy link

backflip commented Feb 20, 2023

@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>

@jorenbroekema
Copy link

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.

@coveralls
Copy link

coveralls commented Feb 21, 2023

Pull Request Test Coverage Report for Build 4265739603

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0%) to 95.455%

Totals Coverage Status
Change from base Build 3616034615: -0.0%
Covered Lines: 248
Relevant Lines: 253

💛 - Coveralls

@eps1lon eps1lon changed the title chore: add an ESM publish step feat: add an ESM publish step Feb 21, 2023
Copy link

@ljharb ljharb left a 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",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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"
},

@eps1lon
Copy link
Member

eps1lon commented Feb 21, 2023

Why do we need to add exports for ES modules with this PR when we haven't supported that before?

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)

Yep, wanted to ship this with 6.0 anyway.

@ljharb
Copy link

ljharb commented Feb 21, 2023

@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 aria-query/lib/esm/index.js. With it, they can import aria-query.

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).

@jessebeach
Copy link
Member

I appreciate the debate here (@ljharb I always value your opinion). I need time to review the arguments for and against.

@KonnorRogers
Copy link
Author

@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.

@jorenbroekema
Copy link

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 "./": "./" or something like that, to keep allowing all filepaths like import foo from "foo/dist/bar/qux.js"; even if not explicitly added as a package entrypoint. I believe this is how I have introduced exports map without causing breaking changes for consumers in the past for some libraries

@KonnorRogers
Copy link
Author

KonnorRogers commented Jan 9, 2024

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.

@jorenbroekema
Copy link

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.

@KonnorRogers
Copy link
Author

@jorenbroekema

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.

@jessebeach
Copy link
Member

jessebeach commented Jan 12, 2024

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 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?

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.

also publish a "modern" bundle
9 participants