Duplicate fireEvent export creates Rollup warnings

[FredKSchott]

• @testing-library/react version: latest
• Testing Framework and version: n/a
• DOM Environment: n/a

Relevant code or config:

https://github.com/testing-library/react-testing-library/blob/master/src/pure.js#L118-L120

// just re-export everything from dom-testing-library
export * from '@testing-library/dom'
export {render, cleanup, act, fireEvent}

What you did:

Bundled this package with Rollup via Snowpack.

What happened:

[snowpack] Conflicting namespaces: ../../node_modules/@testing-library/react/dist/@testing-library/react.esm.js re-exports 'fireEvent' from both ../../node_modules/@testing-library/react/dist/@testing-library/react.esm.js and ../../node_modules/@testing-library/dom/dist/@testing-library/dom.esm.js (will be ignored)

Problem description:

fireEvent is exported twice out of the @testing-library/react package: once directly, and then once indirectly via an export * invocation (see link above). This is invalid ESM that will break in pure ESM environments (ex: Node) and create warnings in others (Snowpack, Rollup, etc). See convo below.

Suggested solution:

Ref: https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export

1. Stop re-exporting @testing-library/dom, let the user import things out of this package themselves as needed.
2. re-export an explicit set of named exports. Harder to maintain.
3. export the custom package fireEvent function as fireReactEvent instead, preventing conflict.

[kentcdodds]

This is a major bummer. I'm not a fan of any of those solutions. I was unaware of this limitation in the spec. Do you have a link to the specification about this?

I'll suggest a 4th solution: Use codegen.macro to generate the exports at build-time so we get the benefits of the 2nd solution without the "hard to maintain" part.

So I think we have a good solution, but I'd like to be sure that the spec really does require this before we go that route.

[kentcdodds]

Oh, also, thank you for bringing this to our attention @FredKSchott :)

[eps1lon]

That really would be a bummer.

I just tried this with node 14.12.0 and had no issues. Though interestingly the order of the exports didn't matter. It always used the "nearest" (dependency tree depth) method.

// run.mjs
import { main } from './index.mjs';

console.log(main());

// index.mjs
export * from './module.mjs';

export function main() {
  return 'index';
}

// module.mjs
export function main() {
  return 'module';
}

node run.mjs always logged index regardless of the order in index.mjs.

I quickly scanned https://tc39.es/ecma262/#sec-exports. The only references to duplicates are:

It is a Syntax Error if the BoundNames of ImportDeclaration contains any duplicate entries.

But that refers to a single statement. So import { main, main } from './module.msj' would be a SyntaxError

It is a Syntax Error if the ExportedNames of ModuleItemList contains any duplicate entries.

That would be

function main() {}

export {main}
export {main}

I was under the impression that Node implements ES Modules by now. What are bundler folks using to test ES module semantics (browsers, node flags etc)?

[barklund]

I do believe this is a violation of the specification – in particular of this rule under section 15.2.1.1:

It is a Syntax Error if the ExportedNames of ModuleItemList contains any duplicate entries.

I understand the specification as follows:

• The list of ExportedNames of ModuleItemList contains all referenced exports across both direct exports and re-exports.
• Star re-exports are expanded to be equivalent to regular re-exports of all exported items of the referenced modules before compiling the list.
• This resulting list cannot contain duplicates.

If we have files foo.mjs and bar.mjs both with just export const a = 1;, all the following scenarios are violations of the above syntax error rule as I understand it:

// star export vs direct export
export * from './foo';
export const a = 2;

// re-export vs direct export
export { a } from './foo';
export const a = 2;

// star export vs re-export
export * from './foo';
export { a } from './bar';

Of these, both node and eslint will only catch the middle one as illegal. The two others are allowed without warnings (but in both, the star export is ignored).

I believe that this must be fixed to conform to the specification.

[FredKSchott]

Thanks everyone for adding detail here! You're right, it looks like Chrome also ignores a conflict. So it sounds like on our end, the Rollup warning that we're seeing is more aggressive than it needs to be for real-world use today. If all other environments support this, we'll probably resolve this on our end more broadly by silencing this warning in Snowpack.

My first impression from all of the spec links above is that this is undefined behavior / not explicitly prohibited.

pinging @MylesBorins as well since I know Node is getting close to an ESM deadline for Node v12 LTS, in case Node's behavior here is unintentional.

[eps1lon]

It is a Syntax Error if the ExportedNames of ModuleItemList contains any duplicate entries.

I think this doesn't error because for export * from './foo' ExportedNames is always empty¹. So whatever you export from ./foo it will not be considered in the ExportedNames of the ModuleItemList and therefore does not lead to conflicts.

¹

ExportFromClause:*
Return a new empty List.

-- https://tc39.es/ecma262/#sec-exports-static-semantics-exportednames

[kentcdodds]

FWIW, if decisions are being made about whether to include this use case in the spec, I can say from real-world standpoints that making this permissible by the spec would be extremely useful and it would be a real shame if the spec explicitly disallowed this. It would likely lead to unexpected breakages the go way beyond Testing Library...

[alexkrolick]

I've seen this syntax cause an error in some tooling before, maybe an older version of Flow or something?

As for impact, we'd have to rethink our re-exports within RTL and our documentation for "custom render" modules.

[eps1lon]

@FredKSchott Is this still an issue in latest versions of snowpack? Shouldn't we move this to the snowpack repository instead? So far snowpack is the first environment (transpiling or implementing ES modules) where we heard of this issue. Considering that my interpretation of the spec allows this pattern and it worked so far for us it seems like this is a snowpack issue not testing-library issue.

[FredKSchott]

Yes! This is safe to close here as valid ESM, and we added a workaround to handle this on our end already.