Add `react-dom` as a peer-dependency

[tryggvigy]

test-utils/ is exported as part of the public API of this module in package.json exports.
It imports from react-dom import { act } from 'react-dom/test-utils'; but react-dom is only declared as a devDependency. This can cause issues in bundlers. I think changing peerDependencies to

"peerDependencies": {
  "react": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0"
+ "react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0"
},

should fix this issue

[tryggvigy]

I found this issue via script I am working on to detect how close a project is to support full ESM by analysing it's dependencies ESM support. react-intersection-observer results in a resolveError due to the above.

Separate issue, but I'm wondering if you think we should also export a ESM version of test utils?

  "exports": {
    "./test-utils": {
      "types": "./dist/test-utils.d.ts",
-     "default": "./dist/test-utils.js"
+     "require": "./dist/test-utils.js",
+     "default": "./dist/test-utils.mjs",
    },
    ".": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.js",
      "default": "./dist/index.mjs"
    }
  },

to fully support consuming this library as ESM

[thebuilder]

test-utils/ is exported as part of the public API of this module in package.json exports. It imports from react-dom import { act } from 'react-dom/test-utils'; but react-dom is only declared as a devDependency. This can cause issues in bundlers. I think changing peerDependencies to

"peerDependencies": {
  "react": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0"
+ "react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0"
},

should fix this issue

Good catch. I suppose it would make sense to add it - You can't really use the library without react-dom.

Alternatively, it could be solved by doing a dynamic import of react-dom instead, so it could fail gracefully if it doesn't exist.
The act is only used in one place, that already handles it as optional.

[thebuilder]

I found this issue via script I am working on to detect how close a project is to support full ESM by analysing it's dependencies ESM support. react-intersection-observer results in a resolveError due to the above.

Separate issue, but I'm wondering if you think we should also export a ESM version of test utils?

  "exports": {
    "./test-utils": {
      "types": "./dist/test-utils.d.ts",
-     "default": "./dist/test-utils.js"
+     "require": "./dist/test-utils.js",
+     "default": "./dist/test-utils.mjs",
    },
    ".": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.js",
      "default": "./dist/index.mjs"
    }
  },

to fully support consuming this library as ESM

Makes sense. 👍 At some point it would also be nice to look into converting the project to type: "module".

[tryggvigy]

Alternatively, it could be solved by doing a dynamic import of react-dom instead, so it could fail gracefully if it doesn't exist.
The act is only used in one place, that already handles it as optional.

I guess then triggerIntersection would have to be async?

[tryggvigy]

I've seen this pattern used in several packages before:

"peerDependencies": { 
  "winston": "> 1.0.0 <= 1.2.10"
},
"peerDependenciesMeta": {
  "winston": {
    "optional": true
  }
}

and then in the CJS/ESM code:

try {
  require('winston')
} catch {
}

But I don't know how ESM friendly this is going forward in the eco-system. I haven't really seen the same pattern with await import(...) probably because it foces the caller to be async as well.

Yes, the code is fine when executed as ESM since the optional peer require is in a try-catch but it's kind of strange and mixes cjs and esm syntax in the same file. But rollup and other popular bundlers handle this. I think in this case they will try to evaluate the dep and if it fails to evaluate they give up and move on since they see it's in a try-catch block. The bundler will have to know how to bundle a module that is mixing esm and cjs though, which is ofc not desired long term as more of the eco-system moves to ESM and kind of contributes to the cjs->esm migration problem. This pattern might also be susceptible to the dual package hazard.

I think the best future-proof solution is to add react-dom as a peer dependency and import it via static import statement

[thebuilder]

It's fine makes sense - I'll make a switch to add it as a peer.

Outputting the test file as .mjs and .cjs is a bit more demanding. Right now it just uses tsc to bundle it, but that will just output the .cjs version.

[thebuilder]

🎉 This issue has been resolved in version 9.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[tryggvigy]

thanks a lot for making this improvement @thebuilder!

[thebuilder]

🎉 This issue has been resolved in version 9.8.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀