fix(plugin-rsc): cjs to esm interop helper doesn't handle native/external cjs import properly

[ErleiX]

Description

• Closes RSC: Internal server error: (0 , html_dom_parser_1.default) is not a function #1072
• Closes RSC: cjs to esm interop helper doesn't handle native/external cjs import properly #1087

This change added a reproduce test for the problem that discovered by issue #1087

The test would fail without this fix and passes with this fix.

When it fails, it is exactly the same error described in issue #1087

The fix is to leverage the Node cjs to esm wrapping behavior, where the wrapped esm namespace of the cjs module is a duplicate of the default export, with additional properties of the default and its named export.

The fix is also considered "safe" and won't break the consumer code, because the fix is inheriently checking that the default is equivalent to the namespace object by comparing all the properties.

[hi-ogawa]

Thanks for PR! Will take a look. Your test case looks good, but can you also explain what's the actual package you hit the issue?

[ErleiX]

Thanks for PR! Will take a look. Your test case looks good, but can you also explain what's the actual package you hit the issue?

sure thing! One of the real cases is that we have a dependency on the "ws" package, where the websocket/websocker-server.js is having exactly the same issue as the test case I added in the PR:

https://github.com/websockets/ws/blob/master/lib/websocket.js
https://github.com/websockets/ws/blob/master/lib/websocket-server.js

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vitejs/plugin-react@1092

npm i https://pkg.pr.new/@vitejs/plugin-react-oxc@1092

npm i https://pkg.pr.new/@vitejs/plugin-rsc@1092

npm i https://pkg.pr.new/@vitejs/plugin-react-swc@1092

commit: 942df8e

[hi-ogawa]

Thanks for reference. In this case, ws package is importing events, but what I normally expect is to externalize ws since they don't need to go through noExternal. RSC plugin does aggresive noExternal only for react dependencies to ensure single react instance, but ws shouldn't be included there. Can you explain why ws ended up transformed by RSC's cjs plugin?

[hi-ogawa]

That said, this fixes genuine bugs so I'm happy to merge this. Can you test pkg.pr.new in #1092 (comment) to confirm if this fix is sufficient for your use case? (as I adjusted the interop slightly)

[ErleiX]

Thanks for reference. In this case, ws package is importing events, but what I normally expect is to externalize ws since they don't need to go through noExternal. RSC plugin does aggresive noExternal only for react dependencies to ensure single react instance, but ws shouldn't be included there. Can you explain why ws ended up transformed by RSC's cjs plugin?

One of the things the team decided is to bundling everything for ssr/rsc to make the server side deployment easier(and potentially better performance without having to resolve the external dependencies at runtime), that's why our vite config is very aggressive with "noExternal". I am happy to learn your perspective on what the benefits or trade-offs are by making dependencies like ws external

[hi-ogawa]

Right, bundling server output is actually legitimate as far as I know, but that can be done at separate build pipeline after vite build. Alternatively, you can also configure noExternal only for vite build, but not on vite dev since inherently transforming all server dependencies can be slower than let node import it as external. This is possible by conditionally returning config via defineConfig(env => env.command === 'build' ? ... : ...) https://vite.dev/config/#conditional-config.

[ErleiX]

Right, bundling server output is actually legitimate as far as I know, but that can be done at separate build pipeline after vite build. Alternatively, you can also configure noExternal only for vite build, but not on vite dev since inherently transforming all server dependencies can be slower than let node import it as external. This is possible by conditionally returning config via defineConfig(env => env.command === 'build' ? ... : ...) https://vite.dev/config/#conditional-config.

that seems to be a better solution, let me try that out, thanks for the suggestion. I also realized that Vite dev and build modes are quite different during debugging of this issue, which makes me wonder what are other gotchas that we'd better to be aware and config dev mode intentionally different, if there are such, please kindly share, thanks!

[ErleiX]

That said, this fixes genuine bugs so I'm happy to merge this. Can you test pkg.pr.new in #1092 (comment) to confirm if this fix is sufficient for your use case? (as I adjusted the interop slightly)

confirming that this fix is sufficient for my use case, please feel free to merge it

[hi-ogawa]

Thanks for contribution!

[hi-ogawa]

The fix is released in v0.5.18 https://github.com/vitejs/vite-plugin-react/releases/tag/plugin-rsc%400.5.18