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

Hooks don't work correctly in a mixed CJS/ESM codebase #4648

Closed
nwalters512 opened this issue Feb 7, 2025 · 13 comments
Closed

Hooks don't work correctly in a mixed CJS/ESM codebase #4648

nwalters512 opened this issue Feb 7, 2025 · 13 comments

Comments

@nwalters512
Copy link

Describe the bug

I have a codebase that uses mixed native ESM and CJS. In reality, this is the situation:

  • My own first-party codebase is 100% ESM.
  • I'm using react-boostrap, which internally uses CJS (they also try to publish native ESM, but did so incorrectly, so I can't use that).
  • I'm using @preact/compat to ensure react-bootstrap uses preact instead of react. I set things up with the instructions here: github.com:nwalters512/preact-esm-repro.git

However, it can be reproduced in isolation without either @preact/compat or react-bootstrap.

To Reproduce

I created a reproduction here: https://github.com/nwalters512/preact-esm-repro

Clone this repository:

git clone [email protected]:nwalters512/preact-esm-repro.git

Install dependencies:

npm install

Build the project:

npm run build

Run the project:

npm start

Observe an error like the following:

TypeError: Cannot read properties of undefined (reading '__H')
    at x (/Users/nathan/git/preact-esm-repro/node_modules/preact/hooks/dist/hooks.js:1:159)
    at m (/Users/nathan/git/preact-esm-repro/node_modules/preact/hooks/dist/hooks.js:1:288)
    at d (/Users/nathan/git/preact-esm-repro/node_modules/preact/hooks/dist/hooks.js:1:257)
    at Object.Bar (/Users/nathan/git/preact-esm-repro/dist/Bar.cjs:7:31)
    at U (file:///Users/nathan/git/preact-esm-repro/node_modules/preact-render-to-string/dist/index.mjs:1:5660)
    at U (file:///Users/nathan/git/preact-esm-repro/node_modules/preact-render-to-string/dist/index.mjs:1:4876)
    at U (file:///Users/nathan/git/preact-esm-repro/node_modules/preact-render-to-string/dist/index.mjs:1:6324)
    at D (file:///Users/nathan/git/preact-esm-repro/node_modules/preact-render-to-string/dist/index.mjs:1:3954)
    at file:///Users/nathan/git/preact-esm-repro/dist/index.js:11:13
    at ModuleJob.run (node:internal/modules/esm/module_job:234:25)

Expected behavior

I would expect to see no error, and see a string logged like the following:

div>Hello, Foo!</div><div>Hello, Bar!</div>

Additional details

I spent a small amount of time digging into this. From what I can tell, this is a classic case of the dual package hazard. hooks/src/index.js relies on internal state, e.g. currentComponent, to function correctly. That file also registers itself with Preact's options object. However, in practice, there are two versions of that file in the published package: node_modules/preact/hooks/dist/index.js and node_modules/preact/hooks/dist/index.mjs. Both of these files end up being loaded in my reproduction: the former by Bar.cts, and the latter by Foo.ts. They can't both register their callbacks into options, so one version of the hooks module won't get the callbacks necessary to set currentComponent, hence the error seen above.

@JoviDeCroock
Copy link
Member

You can use a resolve.alias to ensure that you either choose mjs or cjs which in general is something you want either way else you are duplicating code 😅

@nwalters512
Copy link
Author

Per https://preactjs.com/guide/v10/getting-started#aliasing-in-webpack, that's a Webpack option, right? Neither my linked reproduction nor my actual application are using Webpack, or indeed any bundler at all (at least for SSR).

@JoviDeCroock
Copy link
Member

Sadly that's a limitation then that you won't be able to resolve, in deno and others you can use a concept like import-maps 😅

@nwalters512
Copy link
Author

Is Preact not committed to supporting Node as a runtime? The Preact docs all make it seem like Node is supported, and don't even seem to mention Deno or other runtimes. If Node isn't supported, I'd be happy to open a PR clarifying that in the docs.

@marvinhagemeister
Copy link
Member

This has nothing to do with supporting Node or not and yes we do support Node. The problem is mixing ESM + CJS in a way that you're running into the classic dual package hazard problem. It's a general problem with mixing ESM + CJS and would happen with any other dependency in your setup too. It's not specific to Preact.

The solution to this is to fix your setup. That will also avoid running into the dual package hazard with other libraries in the future potentially.

@nwalters512
Copy link
Author

nwalters512 commented Feb 7, 2025

It's not actually clear to me what's broken with my setup. I'm using Node and Preact exactly as specified in your documentation. I don't think it should be required to switch to a different runtime or use a bundler. Yes, my example is contrived as I shared above, but that's just a minimal reproduction. In practice, it's (sort of, kind of) an issue with using other packages that themselves depends on React/Preact, and whether or not they're publishing CJS or ESM. In practice, I can't control how third parties publish their code. Even if I convince react-bootstrap to publish working ESM, the problem will reappear if I try to use something else from the React/Preact ecosystems that publishes CJS to npm, which is still far more common than not.

There might be some confusion about what the dual-package hazard is. It's generally accepted to be an issue with how packages publish their code, and thus on package maintainers to fix. It is not generally an issue of using a package "incorrectly", which this description of the dual-package hazard calls out.

To be clear, I'm not faulting/blaming you for this. Dual-publishing ESM and CJS can be really really tricky to get right, which is why I wanted to file this issue! I'm even willing to try to open a PR fixing this, as I really like Preact and want to continue using it in my project. Hopefully it's clear how this is an issue with the way Preact is publishing itself, and if you're open to a PR, I'd love to take a shot at it.

@rschristian
Copy link
Member

What's broken is precisely what you noted in your issue: hooks require Preact to be a singleton and yet multiple copies are being loaded. If you cannot author your app in a way to avoid this, then yes, you will need to turn to bundlers and/or other tools that can enforce this for you.

It's generally accepted to be an issue with how packages publish their code, and thus on package maintainers to fix.

Unfortunately that is not the case here. We intentionally provide outputs in a variety of formats to allow users to use Preact in as many configurations and environments as possible and we do so purely, i.e., without facade modules that don't tend to be valid outside of Node (such as ESM importing CJS, this will fall over in the browser). Externalizing state is also not possible for size, perf, and plain UX reasons.

Additionally, package.json#exports cannot be touched outside of a major, altering it in any way is basically guaranteed to cause breakage in some tool out there.

As such, we have to say this is an issue of not using Preact correctly, you must load only one copy at once.

@nwalters512
Copy link
Author

Thanks for the explanation, I guess I'll have to switch to React to take advantage of the broader ecosystem. Would you accept a PR adding this limitation to the docs so that others don't have to discover it independently?

@nwalters512 nwalters512 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 8, 2025
@rschristian
Copy link
Member

Potentially, but we'd frame it instead as a usage error and how users should correct it, not a limitation. Having separate ESM & CJS copies of the same module (& module version) in an app, after all, should never be intentional.

We don't really have a page set up for that at the moment though. IIRC there's an open issue for adding that sort of thing but we haven't yet gotten around to it.

@nwalters512
Copy link
Author

nwalters512 commented Feb 8, 2025

I'd propose something like this, added to https://preactjs.com/guide/v10/getting-started#aliasing-in-node:

Preact is not recommended for use in a mixed CJS/ESM codebase running directly in Node without a bundler, as there's a risk that Preact's singletons could be loaded twice. If your first-party code or any dependencies will try to load both the ESM and CJS versions of Preact, you must use a bundler to alias Preact's imports to always resolve to either the ESM or CJS versions.

This would have been enough to quickly steer me in another direction.

@nwalters512
Copy link
Author

I opened a PR to the docs with the above text: preactjs/preact-www#1234

@developit
Copy link
Member

Wouldn't the simplest solution here be to use a version of Node that fixes the dual package hazard? (the last two majors IIRC)

@nwalters512
Copy link
Author

@developit can you say more about that? I keep up with the Node releases and I don't recall any changes to module resolution that would have resolved this issue. FWIW, I ran my linked reproduction in Node 20, 22, and 23, and I observed the same broken behavior in each.

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

No branches or pull requests

5 participants