testing-library / dom-testing-library

πŸ™ Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.26k stars 466 forks source link

Some dependencies don't support ES modules #995

Closed jimsimon closed 2 years ago

jimsimon commented 3 years ago

Edit 2: Per request, I've simplified this down to a few simple questions and answers.

What did you do? I attempted to use dom-testing-library in a test suite that's be run via Web Test Runner.

What happened? The test runner bombed out complaining about CJS modules that are dependencies of dom-testing-library: aria-query, corejs, and lz-string. There may be more but that's as far as I spent time digging into them. image

What did you expect to happen? I expected the test runner to actually run without dependency errors.

Is there a repro? https://github.com/jimsimon/wtr-dtl/tree/wtr-using-wds (note the branch name)

What have you tried so far? I've tried using the Web Test Runner's CJS support that leverages Rollup's CJS plugin but that did not help. It appears the dependencies causing the errors are not supported by Rollup's CJS plugin.

Is there a proposed solution? Upgrade to versions of these dependencies that support native ES modules or migrate to alternatives that do.

Any other notes? One of the conflicting dependencies (aria-query) was updated in #1058 so that one may no longer be problematic. I haven't had a chance to test it though. I just tried the repro with the latest versions of all dependencies and I'm still having issues.



------------------------------------------------ FIRST EDIT STARTS BELOW -----------------------------------------------------------

Edit: This issue originally started as a request to have dom-testing-library broken down into separate packages to make it easier to consume by non-Jest based test runners. It has since transformed into a discussion about what's preventing dom-testing-libary form working with web-test-runner. In an effort to focus this ticket, I've copied the relevant parts of the conversation below.

First a TLDR: dom-testing-library does not work with web-test-runner. Multiple dependencies used by dom-testing-library do not publish native esm modules and are not compatible with rollup's commonjs plugin (which can be used by web-test-runner). One big offender is aria-query, which may be fixed with #1058 but has not been tested yet. Main repro can be found in https://github.com/jimsimon/wtr-dtl/tree/wtr-using-wds (note the branch name since there are multiple different repros for different attempts to workaround the problem in this repo).

The main problem is that web-test-runner is designed for "buildless" environments using entirely native ESM in the browser. Dom-testing-library has a handful of dependencies that only provide CJS modules that don't play nice in this environment. You can actually use the rollup commonjs plugin for some of these, but some still don't work. Specifically, I recall running into issues with aria-query, corejs, and lz-string. The corejs dependency is some sort of weird prebundled one coming transitively via aria-query.

I'll try to create a sample repo tomorrow and provide a link here, but it's pretty simple to replicate. Create a brand new node project, add web-test-runner and dom-testing-library, write a simple test that uses screen, run the tests via web-test-runner. You'll see web-test-runner just choke and/or throw an error about the CJS deps.

The most success I've had was using snowpack with their web-test-runner plugin in "remote" dependency mode. Unfortunately even that approach errors out.

My suggestion to split the queries out to a separate package was to hopefully reduce the dependency load. A better approach might be to work towards updating to dependencies that also provide native ESM distributions (or remove them altogether).

-- cite>@jimsimon</cite

Does web-test-runner use the module field in package.json?

-- cite>@nickmccurdy</cite

It does as it actually uses https://github.com/rollup/plugins/tree/master/packages/node-resolve under the hood for them. Just for clarity sake, web-test-runner is built on top of web-dev-server, which is what actually handles file transformation.

The main issue seems to be some dependencies that aren't providing ESM distributions. Aria-query in specific is keeping corejs3-runtime as a dependency to support people still on Node 6. There's a ton of discussions about that in https://github.com/A11yance/aria-query/issues/158 and a PR to clean a lot of it up here: https://github.com/A11yance/aria-query/pull/180. So it seems part of the solution here may be to encourage that PR to move forward and then update dom-testing-library accordingly? There are still other dependencies that may need updating as well, but maybe there's some alternatives that provide ESM output available?

-- cite>@jimsimon</cite

I pushed up a couple versions of my demo repo:

Snowpack using remote sources (main branch): https://github.com/jimsimon/wtr-dtl Snowpack using local sources (snowpack-local branch): https://github.com/jimsimon/wtr-dtl/tree/snowpack-local WTR using it's built in Web Dev Server + ESBuild (wtr-using-wds branch): https://github.com/jimsimon/wtr-dtl/tree/wtr-using-wds Hopefully these help. The last link is the one I'd most like to see working and is what spawned this ticket and convo. The Snowpack stuff was an attempt at working around the issue.

Note: The test won't actually pass if you get it to run. That's a different issue/concern regarding being able to pass ShadowRoot to queries. I'm hoping we can address that in a different issue/ticket if we can get this one resolved.

-- cite>@jimsimon</cite

Note that @testing-library/dom itself would not work with native ES modules. But then I see mentions of aria-query which may be fixed with https://github.com/testing-library/dom-testing-library/pull/1058.

So could somebody summarize what the issue is? The issue description is currently missing what is concretely not working (ideally with a repro).

-- cite>@eps1lon</cite

This is the end of the conversation as of this edit. Hopefully this helps!



--------------------------- ORIGINAL ISSUE DESCRIPTION BELOW -------------------------------------------------

Describe the feature you'd like:

I'd love to be able to use the queries provided by testing-library in other testing frameworks. This would allow for a gradual migration to testing-library for codebases with existing test frameworks/runners that aren't compatible with testing-library. One example of a test runner that doesn't play nicely with dom-testing-libary is web-test-runner. Splitting the queries out to a separate package would allow more people to use them and help improve testing in the larger JS ecosystem. This should be doable without any breaking changes, as the existing API's could just delegate to the new package.

Suggested implementation:

Create a new @testing-library/queries package and move all query related code to it.

Describe alternatives you've considered:

The only alternative I can think of would be to reimplement the same queries in an entirely new project. This seems like wasted effort if we can simply extract the ones that testing-library already provides.

Teachability, Documentation, Adoption, Migration Strategy:

There should be no outward facing impact on users as API's would remain backwards compatible. Documentation for the new package would need to be created, and some minor documentation updates might be needed for the main site.

nickserv commented 3 years ago

I'm seeing two related issues here, so I'll split up my response:

Supporting different frameworks

As far as I'm aware, while we primarily focus on Jest support, we still want to support other test frameworks. Testing Library shouldn't necessarily require Jest, it's just a convenient option because of its simple extensible APIs and built in JSDOM support, and it's already popular in the React community.

That being said, I have seen some bugs in some of our packages caused by us assuming a JSDOM environment in Jest, which can cause issues with supporting a real DOM, such as in web-test-runner. I'm assuming that your issue is either related to DOM support or ES module support. Either way, I would appreciate it if you could post an example (preferably one we could run locally) showcasing your specific issues with using DOM Testing Library in web-test-runner.

Using only queries

Because DOM Testing Library doesn't rely on any specific web framework, it works with plain DOM Nodes, so its queries should already be useful in isolation. If you don't want the entire package namespace imported, you have three options:

  1. Import screen to use any query on the entire document: import { screen } from "@testing-library/dom"
  2. Import specific queries to use them on any element: import { getByText } from "@testing-library/dom"
  3. Import queries to use any query on any element: import { queries } from "@testing-library/dom"

From a packaging optimization perspective, I'm not sure how well we support tree shaking, which would let build tools remove other parts of the library that queries don't depend on. If you have a situation where you need to use Testing Library in production but your bundle is too build, I can try to help you look into that if you can post more information, such as the results of running webpack bundle analyzer.

From a package maintenance perspective, separating out our queries into a separate package would require additional infrastructure and manual package dependency bumps (which is a bit trickier to keep track of since we're not using monorepos/workspaces). DOM Testing Library doesn't have too many dependencies, and as long as tree shaking is working properly they can be removed when they're not used anyway. Additionally, I think it's worth noting that our framework specific packages like React Testing Library wrap other DOM Testing Library APIs such as waitFor, so just importing the queries wouldn't be enough.

Example

Let's say you were writing a debugging tool that prints a DOM Node in the console after asking the user for its text content, so you don't care about our other testing utilities such as waitFor, Jest DOM, or React Testing Library. You could import only the queries you want to use and use the DOM Nodes they return.

import { screen } from "@testing-library/dom";

const text = prompt("Type in the text of a Node to get");

if (text) {
  const node = screen.getByText(text);
  console.log(node);
} else {
  console.log("Node not found");
}
jimsimon commented 3 years ago

@nickmccurdy Thanks for the elaborate response. I actually tried the approach that you suggested before creating this ticket, but it still doesn't work. The main problem is that web-test-runner is designed for "buildless" environments using entirely native ESM in the browser. Dom-testing-library has a handful of dependencies that only provide CJS modules that don't play nice in this environment. You can actually use the rollup commonjs plugin for some of these, but some still don't work. Specifically, I recall running into issues with aria-query, corejs, and lz-string. The corejs dependency is some sort of weird prebundled one coming transitively via aria-query.

I'll try to create a sample repo tomorrow and provide a link here, but it's pretty simple to replicate. Create a brand new node project, add web-test-runner and dom-testing-library, write a simple test that uses screen, run the tests via web-test-runner. You'll see web-test-runner just choke and/or throw an error about the CJS deps.

The most success I've had was using snowpack with their web-test-runner plugin in "remote" dependency mode. Unfortunately even that approach errors out.

My suggestion to split the queries out to a separate package was to hopefully reduce the dependency load. A better approach might be to work towards updating to dependencies that also provide native ESM distributions (or remove them altogether).

Edit: fat fingered the close issue button on my phone

nickserv commented 3 years ago

Does web-test-runner use the module field in package.json? We have one, but I'm not sure if the issue is a lack of support in web-test-runner, or that our ES module build still has some CJS module dependencies. If the issue is needing dependencies to be bundled, there's also a browser field we could theoretically start using.

Ideally I think the best solution would be supporting ES module tools as well as we can, but I recognize there can be a lot of complicated inconsistencies between them, and that our dependencies could cause additional issues. If we decided not to go this route and split the packages, we'd be trading ES module maintenance for separate package maintenance, and limiting which parts of Testing Library could be used with similar ES module based tools.

alexkrolick commented 3 years ago

FYI aria-query is a dependency of getByRole anyways, so pulling out the queries package will not fix the module format issue for this use case

jimsimon commented 3 years ago

Does web-test-runner use the module field in package.json?

It does as it actually uses https://github.com/rollup/plugins/tree/master/packages/node-resolve under the hood for them. Just for clarity sake, web-test-runner is built on top of web-dev-server, which is what actually handles file transformation.

...but I'm not sure if the issue is a lack of support in web-test-runner, or that our ES module build still has some CJS module dependencies. If the issue is needing dependencies to be bundled, there's also a browser field we could theoretically start using.

The main issue seems to be some dependencies that aren't providing ESM distributions. Aria-query in specific is keeping corejs3-runtime as a dependency to support people still on Node 6. There's a ton of discussions about that in https://github.com/A11yance/aria-query/issues/158 and a PR to clean a lot of it up here: https://github.com/A11yance/aria-query/pull/180. So it seems part of the solution here may be to encourage that PR to move forward and then update dom-testing-library accordingly? There are still other dependencies that may need updating as well, but maybe there's some alternatives that provide ESM output available?

nickserv commented 3 years ago

I'm trying to replicate this, but importing DOM Testing Library (with no additional config) seems to give me an unrelated error:

// test.js
import "@testing-library/dom";
$ web-test-runner test.js
test.js:

 🚧 Browser logs:
      TypeError: Failed to resolve module specifier "@testing-library/dom". Relative references must start with either "/", "./", or "../".

 ❌ Could not import your test module. Check the browser logs or open the browser in debug mode for more information. 

Chrome: |β–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆ| 1/1 test files | 0 passed, 0 failed

Error while running tests.

Also, I can't seem to find where web-test-runner is using esbuild, outside of an optional plugin. Has this dependency changed since?

jimsimon commented 3 years ago

Sorry, been a busy week so I haven't gotten the example repo up yet.

What command and config are you using for WTR? I believe you need to specify --node-resolve on the CLI or set nodeResolve: true in the config file in order to use node style imports.

I think you're right that esbuild is an optional dependency. I don't believe I was using it.

jimsimon commented 3 years ago

I'll try to push the example up tonight if it'll help

nickserv commented 3 years ago

Ah sorry, I didn't realize --node-resolve was required. Now I'm getting this, which seems more like your issue:

$ web-test-runner test.js --node-resolve
test.js:

 🚧 Browser logs:
      SyntaxError: The requested module './../../../../@babel/runtime/regenerator/index.js' does not provide an export named 'default'

 ❌ Could not import your test module. Check the browser logs or open the browser in debug mode for more information. 

Chrome: |β–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆβ–ˆ| 1/1 test files | 0 passed, 0 failed

Error while running tests.

Inspecting my dependencies, it seems like DOM Testing Library itself is also including @babel/runtime, so there's probably further work for us to do to support this:

$ npm ls @babel/runtime
web-test-runner-example
└─┬ @testing-library/dom@8.1.0
  β”œβ”€β”€ @babel/runtime@7.14.8 
  └─┬ aria-query@4.2.2
    └── @babel/runtime@7.14.8  deduped

Reproduction

jimsimon commented 3 years ago

That's similar to what I'm getting. I just got back to my laptop and checked, and I am using esbuild which should get you past that error (I think).

I pushed up a couple versions of my demo repo:

Hopefully these help. The last link is the one I'd most like to see working and is what spawned this ticket and convo. The Snowpack stuff was an attempt at working around the issue.

Note: The test won't actually pass if you get it to run. That's a different issue/concern regarding being able to pass ShadowRoot to queries. I'm hoping we can address that in a different issue/ticket if we can get this one resolved.

TimvdLippe commented 3 years ago

I found this issue via the aria-query issue I filed. Are there maybe alternatives to aria-query that do support modern Node versions without issues? In an extreme case, temporarily forking might be necessary to unblock this issue?

sijakret commented 3 years ago

1up. also not getting the library to work with web-dev-server. also when trying to use specific rules in rollup commonjs plugin to load the es5 libs i get stuck at some point :(

eps1lon commented 2 years ago

Note that @testing-library/dom itself would not work with native ES modules. But then I see mentions of aria-query which may be fixed with https://github.com/testing-library/dom-testing-library/pull/1058.

So could somebody summarize what the issue is? The issue description is currently missing what is concretely not working (ideally with a repro).

sijakret commented 2 years ago

Fyi: for now, we have resorted to creating a commonjs build via webpack that can be consumed with the corresponding rollup plugin.

jimsimon commented 2 years ago

@eps1lon I've attempted to update the issue description with a "current status" summary and the relevant quotes from the conversation. Hopefully this is helpful! Let me know if there's anything else I can do to further clarify the problem.

eps1lon commented 2 years ago

@eps1lon I've attempted to update the issue description with a "current status" summary and the relevant quotes from the conversation. Hopefully this is helpful! Let me know if there's anything else I can do to further clarify the problem.

Could you shorten the issue description? It's a really long text with no clear description what you did, what happened and what you expected instead.

jimsimon commented 2 years ago

@eps1lon I've added another edit to the top of the issue description. I've avoided removing the original issue description and edit since it would likely make most of the following conversation hard to follow. You can probably ignore the original description and edit at this point.

eps1lon commented 2 years ago

I'm having a hard time understanding why this is an issue on our side? So far the guarantee was always that you can import CJS from ESM. So everything should be working without any work on our side.

I suggest opening an issue on the side of the bundler/test runner you're using so that we can coordinate which library should do what.

sheremet-va commented 2 years ago

I'm having a hard time understanding why this is an issue on our side? So far the guarantee was always that you can import CJS from ESM. So everything should be working without any work on our side.

@eps1lon this is not actually true. Node cannot guarantee safe importing. I am trying to create a loader that will allow Node to understand module field, and I was quite successful with it. However, Node doesn't allow importing named exports, if it didn't find them (they were defined not in a global namespace, or dynamically). Currently, I am getting this error from this package:

SyntaxError: Named export 'compressToEncodedURIComponent' not found. The requested module 'lz-string' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'lz-string';
const { compressToEncodedURIComponent } = pkg;

I guess the solution would be to use import { compressToEncodedURIComponent } = require('lz-string'). That way transpiler can understand that you use CJS package, and will transform it into:

const require = createRequire(import.meta.url)
const { compressToEncodedURIComponent } = require('lz-string')
eps1lon commented 2 years ago

@eps1lon this is not actually true. Node cannot guarantee safe importing.

Could you file an issue against NodeJS in that case please? Their documentation clearly states you can import CommonJS from ES modules:

An import statement can reference an ES module or a CommonJS module.

-- https://nodejs.org/api/esm.html#import-statements

@sheremet-va I think in this specific case it has more to do with the way we import lz-string. Could you create a minimal, cloneable reproduction that reproduces the issue you linked?

sheremet-va commented 2 years ago

Could you file an issue against NodeJS in that case please? Their documentation clearly states you can import CommonJS from ES modules:

An import statement can reference an ES module or a CommonJS module.

It doesn't contradict documentation. You can import CJS module, but you might not be able to use named exports, if they are not statically analysable:

Live binding updates or new exports added to module.exports are not detected for these named exports.

The detection of named exports is based on common syntax patterns but does not always correctly detect named exports. In these cases, using the default import form described above can be a better option.

Named exports detection covers many common export patterns, reexport patterns and build tool and transpiler outputs. See cjs-module-lexer for the exact semantics implemented.

-- https://nodejs.org/api/esm.html#commonjs-namespaces

If it cannot detect a named export, it will throw an error that I described.

I think in this specific case it has more to do with the way we import

Yes, technically lz-string cannot be exported as { namedExport }, since its exports are not statically analysable (it just puts an object on module.exports - as you can see from the quote above these bindings are not detectable).

I made a small reproduction for you, using stackblitz: https://stackblitz.com/edit/node-fpxnof?file=index.mjs

There are several ways to fix this:

sheremet-va commented 2 years ago

This is usually a non-issue, because bundlers don't run code. But if ecosystem wants to move forward with ESM, I think it would be in community interest to support it.

plusgut commented 2 years ago

I would like to add, that it would be great to be able to run dom-testing-library not just inside node.js but in the browser natively without a bundler. As far as i can see, lz-string is the only thing stopping that from happening.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 8.16.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

thebuilder commented 2 years ago

Running React with vitest, and after upgrading to 8.16.1 a bunch of my tests now fail with act() warnings. Not sure of all the triggers, but I've narrowed one issue down to having a useQuery hook that's called, so it performs an async data fetch.