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

Native ESM import problem 'with use-sync-external-store' #2191

Closed
1 task
inukshuk opened this issue Jul 29, 2024 · 8 comments
Closed
1 task

Native ESM import problem 'with use-sync-external-store' #2191

inukshuk opened this issue Jul 29, 2024 · 8 comments

Comments

@inukshuk
Copy link

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

  • React: 18
  • ReactDOM: 18
  • Redux: 5.0.1
  • React Redux: 9.1.2

What is the current behavior?

The module 'use-sync-external-store' published at the moment (v1.2.2) is a CommonJS module. When importing a CommonJS module into an ESM module with import, e.g. using Node's ESM loader named imports will not work. Because of this, using react-redux using native ESM loader currently produces an error like this:

The requested module 'use-sync-external-store/with-selector.js' 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 'use-sync-external-store/with-selector.js';
const { useSyncExternalStoreWithSelector } = pkg;"

This is probably not an issue in most setups because the modules are loaded via some compiler/bundler but it's an issue when using Node's native module loader.

What is the expected behavior?

If the import here and probably (?) here could be changed to rely on the default export instead then the import should work.

Which browser and OS are affected by this issue?

Node.js

Did this work in previous versions of React Redux?

  • Yes
@timdorr
Copy link
Member

timdorr commented Jul 29, 2024

This will be fixed by facebook/react#25231. It's a bit of a pointing fingers thing. We're not doing anything incorrect, uSES is doing things technically wrong. It sounds like there could be some movement soon, but I'll leave this open to fix on our end for now.

@phryneas
Copy link
Member

use-sync-external-store/with-selector.js has no default export - the syntax suggestion given there would work in some environments and absolutely fail in a lot of others. I fear that's not gonna be the solution :/

We might get away with keeping a require call in the ESM files here? 🤔

@inukshuk
Copy link
Author

Thanks!

Out of curiosity, in which environments is the CommonJS module.exports object not treated as the default export when importing into an ESM module?

Speaking only for Node.js require is not available in ESM modules (outside of using module.createRequire) so for tricky situations the best bet is often a dymaic import -- but of course that's really ugly and probably better to wait for the upstream package to be updated.

@phryneas
Copy link
Member

phryneas commented Jul 30, 2024

I might actually have gotten it the wrong way round, my bad. ESM can import CJS, but still needs the import keyword for that.

I'm still not convinced that the "default" syntax suggested here would work in all cases, though :/

Is this really an error message and not just a warning? The wording ("may") weirdly sounds like it isn't even sure if the original statement might even work or not.

@aryaemami59
Copy link
Contributor

@phryneas Yeah I'm kind of confused too, because right now if you have a file like someFile.mjs that contains something like this:

import { useSyncExternalStoreWithSelector } from 'use-sync-external-store/with-selector.js'
console.log(useSyncExternalStoreWithSelector)

You can run it by:

node someFile.mjs

and you will get:

[Function: useSyncExternalStoreWithSelector]

which is fine. You can also have this in a someFile.cjs file:

const { useSyncExternalStoreWithSelector } = require('use-sync-external-store/with-selector.js')
console.log(useSyncExternalStoreWithSelector)

and you will again get the same thing so I'm not sure exactly what the issue is.

@inukshuk
Copy link
Author

inukshuk commented Aug 1, 2024

@aryaemami59 oh, you're right the import works fine with the latest Node.s loader. For some reason, I can only reproduce this when I import react-redux, specifically, the error happens in dist/react-redux.mjs line 3. This is in an Electron renderer process using the Node.js native module loader. The odd thing is that the import works fine in this environment if I import it directly. I'll debug this further to figure it out, but the syntax should be fine, so I'll close this issue here with apologies for the noise.

@phryneas it's thrown as a SyntaxError by Node's ESM loader (in ModuleLoader.import) so it's really a hard error that stops everything at load time. I agree that the wording is extremely odd. I've seen similar errors with experimental JSON module import so I wasn't too surprised. However the error message is extremely baffling given that the syntax itself seems to work fine in general. I'll post here if I can make more sense of it.

@inukshuk inukshuk closed this as completed Aug 1, 2024
@inukshuk
Copy link
Author

inukshuk commented Aug 1, 2024

OK I think I understand a little bit better why this is happening. As described here the Node.js loader will always import the CJS module.exports as the default export. In addition, it will try to detect and make available the properties of module.exports as named exports based on static analysis. It's not guaranteed that the static analysis detects all exported properties which explains the vague wording in the error message.

In this particular case the static analysis of use-sync-external-store/with-selector.js should detect the named export and therefore the import syntax here works fine in theory. It also works fine for me using the import statement in my entry file. However, curiously, when I use the import statement ('react-redux' in my case) in a different file which is not imported statically, but later on with the import method, then I run into the syntax error. My theory is that this is due to an issue with the ESM loader in Electron renderer processes, that for some reason the static analysis of the imported file fails or is skipped when the import is delayed. I can work around the issue by importing 'react-redux' right away. Then the later dynamic import will also work, presumably because the static analysis already happened and is still cached. (If I do this the loader will fail at the next CJS module it encounters...)

So yes, there's definitely not an issue here, it's an issue with the ESM loader in Electron. Sorry for the noise! But one thing to take away is that if you encounter this vague SyntaxError it likely means that the static analysis failed.

@inukshuk
Copy link
Author

inukshuk commented Aug 5, 2024

Just leaving this here in case somebody hits a similar issue with the ESM loader in Electron: the issue happens if you use a dynamic import after the page has loaded and your CSP prohibits 'unsafe-eval'.

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

4 participants