A11yance / aria-query

Programmatic access to the ARIA specification
Apache License 2.0
275 stars 43 forks source link

also publish a "modern" bundle #48

Closed smeijer closed 3 months ago

smeijer commented 4 years ago

Aria-query is currently only publishing the transformed assets to npm. This has 2 issues:

1) Modern browsers are using transforms when it's not required 2) The bundle size is larger than strictly required 3) This project can not be included in any way when using parcel as bundler. See https://github.com/parcel-bundler/parcel/issues/4692

I believe the project would benefit from having a modern bundle as well.

Looking at the build stack being used, I think using microbundle for building the modern bundles would be a good fit, and easy to implement.

jessebeach commented 4 years ago

Good suggestion. I've never used microbundle. Let me do some research on it.

smeijer commented 4 years ago

Regarding microbundle, any other bundler would also be fine of course. I just suggested that one, because it's dead simple. So it would safe you the pain from maintaining a webpack config.

jessebeach commented 4 years ago

@smeijer I'm curious how you're using this module in a browser. Maybe the answer to the issue is a static resource that doesn't use Map?

smeijer commented 4 years ago

@jessebeach, I'm using this library indirect, via @testing-library/dom. Which I'm importing entirely in https://testing-playground.com.

If my project would be the only reason to make that modern bundle, then please don't waste your time 🙂. I have it working with parcel 2 (alpha).

I'm not sure about why it works with 2 and not with 1 though. Maybe a tree shaking thing?

jessebeach commented 3 years ago

If my project would be the only reason to make that modern bundle, then please don't waste your time 🙂. I have it working with parcel 2 (alpha).

It would be :) Feel free to propose the PR, though, in the future!

jimsimon commented 1 year ago

I'm also using this library indirectly via @testing-library/dom and having build issues due to mixing ESM and CJS. It'd be great if this project produced both types of builds (or upgraded to output ESM). In my case, I'm having issues with it when using Rollup via Web Test Runner, even when using Rollup's commonjs plugin.

SyntaxError: The requested module '/__wds-outside-root__/4/node_modules/@testing-library/dom/node_modules/aria-query/lib/index.js?wds-import-map=0' does not provide an export named 'elementRoles'
jessebeach commented 1 year ago

@jimsimon I'm happy to incorporate such a build step. Could you propose the PR for it?

KonnorRogers commented 1 year ago

I'm currently running into this.

 🚧 Browser logs on Chromium:
      SyntaxError: The requested module './../../../../../../aria-query@5.1.3/node_modules/aria-query/lib/index.js' does not provide an export named 'elementRoles'

Using web-test-runner + @testing-library/dom.

Happy to submit a PR for Rollup if thats okay.

jessebeach commented 1 year ago

@KonnorRogers thank you for offering the PR. I'm happy to review it. I don't know offhand how to fix this without doing some research, so if you know what to do, let's start there. I see the property elementRoles on the exports object, but I guess that isn't accessible in a browser context?

KonnorRogers commented 1 year ago

@jessebeach correct. That “exports” object doesn’t exist in ESM world. I’ll be adding a Rollup config which uses your existing babel config and will be adding a couple extra keys, easier to PR than to explain in some ways, and I’ll highlight the key points.

but I plan to keep it backwards compatible and not change any existing paths.

ljharb commented 1 year ago

Why is a bundle needed? The best practice remains for an app to bundle, and never for a package to do so.

KonnorRogers commented 1 year ago

@ljharb the bundle is more about using ESM compatible syntax.

It’s not “needed” per se, but aria-query will keep popping up if it doesn’t ship ESM compatible entrypoints.

I don’t know that aria-query is being used in actual browsers (I could be wrong) but rather for Node.

node is in this weird state of flux with supporting both CJS and ESM.

That best practice of bundling in the app is general guidance for Browsers, Node is different.

https://jasonformat.com/enabling-modern-js-on-npm/

The jury is even still out on how best to handle bundling / unbundling for NPM. Fwiw, my PR really just uses what exists today and just makes an ESM compatible version. It doesn’t actually change anything that the library is doing today. It uses the same Babel config, keeps the folder structure the same, it just adds a “module” key and a “/esm” directory.

https://github.com/A11yance/aria-query/pull/485

I ended up having a workaround so feel free to close it, I was just driving by and forked it for my own stuff but figured I’d contribute back.

ljharb commented 1 year ago

Why will it keep "popping up"? CJS is an ESM-compatible entrypoint.

node will support both CJS and ESM for the foreseeable future - CJS isn't "legacy" and is highly unlikely to ever go away.

KonnorRogers commented 1 year ago

@ljharb *keep popping up when people try to use @testing-library/dom in an ESM context. Yes there is workarounds and I managed to work around it in my @web/test-runner project by using a canary build + common js transform.

🤷‍♂️ not here to convince anybody. If I hadn’t come across this issue I wouldn’t have even PRed it.

ljharb commented 1 year ago

That sounds like a flaw in RTL then, or, with the bundler you're using.

A working node module bundler silently handles any JS format node supports (CJS and ESM) as well as provides the proper globals to it. If you're using a bundler that doesn't, that's your problem.

jimsimon commented 1 year ago

Not everyone is using a node based bundler or running on a platform that supports CJS (e.g. a browser). Web Test Runner itself is designed for "buildless" development experiences using native ESM in browsers. The future of the web is ESM and it makes sense for packages to support ESM syntax natively. The node team has even recognized this by giving us the metadata, tools, and compatibility to promote native ESM syntax.

@KonnorRogers's PR is addressing a real problem that real people and real companies are experiencing.

Correct me if I'm wrong, but I don't think @KonnorRogers meant "bundle" as in a concatenated and minified production-ready file. I interpreted it as offering the existing files with a transform that outputs ESM syntax.

ljharb commented 1 year ago

@jimsimon "buildless" is, and will likely always be, untenable. Native ESM syntax in node doesn't help that, because core modules, bare specifiers, and the filesystem exist. "bundle" has a definition, and it's something packages should never do (it's what rollup is for, for example); "transpile" would be automatically converting one format to another (which is what babel is for).

jimsimon commented 1 year ago

I think we can agree to disagree on whether "buildless" is untenable. There are projects out there that already use buildless development processes. Rollup is also perfectly capable as a transpiler/transformer tool. The PR that @KonnorRogers put up is using rollup as a means of running babel (and a few other plugins). ~It also extends the rollup build that this project already has in place.~ Edit: I misspoke here. The rollup build is new.

I really don't understand why someone would be against publishing an ESM compatible version of a library for environments that don't support CJS. It doesn't impact anyone using node/CJS/whatever, and it expands compatibility for the library. There is literally zero downside here.

ljharb commented 1 year ago

Environments that don't support CJS - like browsers - already have a decades-old, battle-tested solution: bundlers. Packages don't need to add complexity, and potential sources of bugs, to account for this.

jimsimon commented 1 year ago

Actually, I'm going to partially agree with you here. Packages should just output one format by default and then projects can use bundlers to switch to other formats if they want! It's not my project, but I think that sounds like a great solution to make everyone happy. Switching this project to output only ESM would keep a single build, have the broadest possible platform support, and reduce the likelihood of bugs since it's using the official TC39 standard for modules!

I think accomplishing this would just require dropping the babel build entirely (I haven't looked too deeply into it though).

ljharb commented 1 year ago

Unfortunately, "ESM-only" would mean that CJS clients can't use it, so that'd be a user-hostile move. Packages should be CJS-only to be maximally compatible.

I'm on TC39, and imo the fact that ESM is part of the standard is irrelevant - using it offers virtually no upsides and a ton of downsides. Stick with CJS.

jimsimon commented 1 year ago

They can just use a bundler :) See how this conversation can be flipped around. I made that last comment to make a point.

The correct solution is to provide both. Doing so future-proofs the library, maximizes reach, and makes everyone happy. The chance of bugs resulting from doing so is practically 0. I have a lot of trust in mature tools like rollup and babel properly transpiling ESM to CJS (note, the source files are already using ESM here).

Oh, and I could care less if you're on TC39, though I do find it funny you're in the group that standardized ESM but then turn around and tell everyone not to use it.

ljharb commented 1 year ago

A bundler or transpiler is always required; providing native ESM doesn't absolve you of that. You'll always need a build process to generate import maps, at least - buildless is impossible.

As for rollup, its heuristics violate the spec, otherwise it wouldn't provide any benefit over browserify or webpack.

backflip commented 1 year ago

As commented in https://github.com/A11yance/aria-query/pull/485#issuecomment-1437403612 we can absolutely use this library as an ESM module in every current browser without any build step. Or did you have a different usage in mind, @ljharb?

ljharb commented 1 year ago

@backflip I don't consider exposing node_modules paths to the web a viable path forward, for hopefully obvious security reasons, and I don't see any other alternative to avoid that than a build step.

backflip commented 1 year ago

@ljharb what if I tell my nginx to expose node_modules/aria-query/lib as libs/aria-query/lib in this specific example? So only exposing the parts of node_modules we are interested in exposing?

ljharb commented 1 year ago

Sure - but the way you'd scalably and statically figure out which things that includes, to set up your nginx config? That's a build process.

backflip commented 1 year ago

@ljharb, I would argue it is a rather long way from configuring a server which parts of node_modules to statically serve to setting up (and maintaining) something like Webpack to create an ES module from CJS.

jorenbroekema commented 4 months ago

Unfortunately, "ESM-only" would mean that CJS clients can't use it, so that'd be a user-hostile move. Packages should be CJS-only to be maximally compatible.

I'm on TC39

Okay.... interesting.

CJS files can import ESM files just fine (albeit asynchronously):

const mod = await import('./some-esm-file.mjs');

ESM files can import CJS files just fine too:

import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);

const siblingModule = require('./some-cjs-file.cjs'); 

Interoperability is fairly decent between both syntaxes, in Node.

The problem, as others have pointed out, is that when you're inside a browser environment, you can't do the createRequire trick to import CJS files and you will be reliant on a bundler/compiler to convert it for you.

You can think all you want that the movement towards "buildless" development is an impossible mission, but the fact is that thousands of projects are already doing this successfully by using the latest browser APIs such as import maps to resolve bare module specifiers and using browser shims for packages such as path, fs, util, stream, buffer, the list goes on. By now the community has proven this is anything but untenable.

I don't really understand why there is so much resistance to getting the ecosystem moving in this direction tbh:

At the very least you could dual CJS/ESM publish if you really insist on not causing breaking changes for CJS users that might have to asynchronously import your ESM package.

There's plenty of us that would contribute this change but I think the main showstopper is you @jessebeach because you seem quite hesitant on giving the green light, and we don't want to spend time to raise a PR that's just going to be stopped by @ljharb