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

Fix: remove server.browser.js in favor of export mapping to server #6

Merged
merged 1 commit into from Jun 6, 2022

Conversation

lfre
Copy link
Contributor

@lfre lfre commented Jun 5, 2022

The browser subpath is for browser/client bundles. The server.browser path from Next.js is requested in Node.

Adding the corresponding export mapping to the same server file addresses the issue in Next.js 12.1.6.

@lfre
Copy link
Contributor Author

lfre commented Jun 6, 2022

@JoviDeCroock I'd appreciate it if you can take a look at this. The current server.browser.js file/reference does not fix the issue in Next 12.1.6.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jun 6, 2022

This looks wrong 😅 you both remove the contents of the file as well as introduce a new mapping. Next should not take browser for a node environment.

Also that file has no side-effects and no browser-specific behavior so should not cause issues, this might be a next issue instead

@lfre
Copy link
Contributor Author

lfre commented Jun 6, 2022

@JoviDeCroock I'm not following what you meant. There's no need for an additional file since the expectations from the server.browser.js are the same exports as server, so the mapping is all it's needed. Next 12.1.6 is requesting the file in Node, not through webpack, meaning the browser subpath will not match, and not find the current server.browser.js file.

This behavior was masked by this merged PR in Next vercel/next.js#36749, but the issue remains that if you do a require('react-dom/server.browser'). It will throw an error.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jun 6, 2022

but why would they use require('server.browser') that's just wrong 😅 we have export-maps now - i.e. what's happening here is a bit counterintuitive from what is expected.

EDIT: I mean I get why this fixes the problem but it makes me feel like we are fighting the intended usage

@lfre
Copy link
Contributor Author

lfre commented Jun 6, 2022

@JoviDeCroock Export maps are behaving as expected, though. The browser field will not clear in Node, and the path would have to be server not server.browser. I don't think there's a current expectation to load that file in the browser, if that's the case, then a browser field would need to get added to server.browser mapping.

@JoviDeCroock
Copy link
Member

Heh, I honestly really don't like this but if they intend to use it that way this is mainly used for next either way 🤷‍♂️

@JoviDeCroock JoviDeCroock merged commit 2eed8a3 into preactjs:main Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants