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

also publish a "modern" bundle #48

Open
smeijer opened this issue Jun 5, 2020 · 28 comments
Open

also publish a "modern" bundle #48

smeijer opened this issue Jun 5, 2020 · 28 comments

Comments

@smeijer
Copy link

smeijer commented Jun 5, 2020

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 Error in production bundle: Incompatible receiver, Map required parcel-bundler/parcel#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
Copy link
Member

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

@smeijer
Copy link
Author

smeijer commented Jun 12, 2020

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
Copy link
Member

@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
Copy link
Author

smeijer commented Jun 13, 2020

@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
Copy link
Member

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
Copy link

jimsimon commented Oct 4, 2022

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
Copy link
Member

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

@KonnorRogers
Copy link

KonnorRogers commented Feb 19, 2023

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
Copy link
Member

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

@jessebeach jessebeach reopened this Feb 20, 2023
@KonnorRogers
Copy link

KonnorRogers commented Feb 20, 2023

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

ljharb commented Feb 20, 2023

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

@KonnorRogers
Copy link

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

#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
Copy link

ljharb commented Feb 20, 2023

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
Copy link

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

ljharb commented Feb 20, 2023

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
Copy link

jimsimon commented Feb 20, 2023

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
Copy link

ljharb commented Feb 20, 2023

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

jimsimon commented Feb 20, 2023

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
Copy link

ljharb commented Feb 20, 2023

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
Copy link

jimsimon commented Feb 20, 2023

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
Copy link

ljharb commented Feb 20, 2023

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
Copy link

jimsimon commented Feb 20, 2023

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
Copy link

ljharb commented Feb 20, 2023

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
Copy link

As commented in #485 (comment) 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
Copy link

ljharb commented Feb 20, 2023

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

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

ljharb commented Feb 20, 2023

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
Copy link

backflip commented Feb 21, 2023

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

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 a pull request may close this issue.

6 participants