react-markdown doesn't work in SSR dev

[cyco130]

Describe the bug

react-markdown fails with ReferenceError: module is not defined when I try to import it from a module loaded with ssrLoadModule. It seems to be caused by mixing ES modules and CJS modules in the transitive dependencies. Everything works as expected when I add "type": "module" to package.json.

Reproduction

• Clone this repo and install dependencies: https://github.com/cyco130/vite-react-markdown-issue
• Run node ssr-module.mjs and observe that react-markdown works correctly when run directly.
• Run node index.cjs and observe loading with SSR fails.
• Add "type": "module" to package.json to fix it.

System Info

System:
    OS: Linux 5.4 Linux Mint 20.2 (Uma)
    CPU: (8) x64 Intel(R) Core(TM) i7-3630QM CPU @ 2.40GHz
    Memory: 365.33 MB / 15.53 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.18.1 - /usr/local/bin/node
    Yarn: 1.22.11 - ~/.npm-global/bin/yarn
    npm: 7.24.1 - ~/.npm-global/bin/npm
  Browsers:
    Chrome: 96.0.4664.45
    Firefox: 94.0
  npmPackages:
    vite: ^2.7.0-beta.9 => 2.7.0-beta.9

Used Package Manager

npm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[benmccann]

The code has:

import ReactMarkdown from "react-markdown";

In ssrImport there's a check:

        if (dep[0] !== '.' && dep[0] !== '/') {
            return nodeImport(dep, mod.file, resolveOptions);
        }

It's encountering:

/node_modules/react-markdown/index.js
/node_modules/react-markdown/lib/react-markdown.js

So this is not being treated as external. I think if we did it would fix this. @brillout has suggested several times that we treat all dependencies as external by default

[benmccann]

This seems suspect to me:

vite/packages/vite/src/node/ssr/ssrExternal.ts

Line 144 in 1f91cdb

if (pkg.type === 'module' || esmEntry.endsWith('.mjs')) {

pkg.type is referring the the package.json of the reproduction, but I think it should refer to the dependency's package.json instead?

This was added in #5197. CC @natemoo-re

[benmccann]

Perhaps we should revisit the suggestion in this comment: #5544 (comment). I.e. rather than only externalizing packages that meet a certain specification, we should reverse that check, externalize by default, and only transform in certain cases where it's necessary. And I'm not sure it's ever necessary. I had been wondering about things like AMD or SystemJS, but I don't think Rollup works with those anyway? Because I realized that even to handle CommonJS, there's @rollup/plugin-commonjs. But there's no such plugin for AMD or SystemJS that I'm aware of. I've never encountered packages for bundling being distributed in those formats

[cyco130]

rather than only externalizing packages that meet a certain specification, we should reverse that check, externalize by default, and only transform in certain cases where it's necessary.

Isn't it actually necessary to transform in this case, at least in production build? If we don't transform, the CJS build will try to require an ESM module which will break the build. It doesn't break the dev for some reason, but build doesn't work.

[brillout]

@cyco130 It's expected and a good thing, see #4340 (comment)

@benmccann

. I.e. rather than only externalizing packages that meet a certain specification, we should reverse that check, externalize by default, and only transform in certain cases where it's necessary.

👍 And it makes reasoning about ssrExternal.ts a lot easier.

[Niputi]

I dont think this issue should be added as a 2.7 goal. its not a breaking change and not a regression

[benmccann]

Hmm. This particular report was broken before 2.7, but the line that's breaking currently was added in 2.7, so I'm not 100% sure there wouldn't be a regression for other libraries given that there is newly added code that's incorrect. In any case, this shouldn't be hard to get into 2.7 since I've sent a fix for it already: #5903

[patak-dev]

I agree with Niputi here, the fix looks a bit involved for this point of the beta. Let's try to fix the other two remaining regressions and do the release (https://github.com/vitejs/vite/milestone/4)

[brillout]

@patak-js I will have a look at these 2 regressions. (But let me finish 2 highest prio vite-plugin-ssr things I need to finish today. Hopefully I'll have some spare time for this later today.)

[benmccann]

I still think it's incorrect to say there's no regression here in 2.7. While the reproduction the reporter shared doesn't demonstrate a regression, if you read the code it sure looks like there's one and I'd expect we could create a reproduction.

The newly introduced code says the package should be externalized if pkg.type === 'module' but is reading the wrong package.json. I think this means that if the user's project is of "type": "module" then all packages will be externalized. And per the discussion in #5903 it sounds like we don't want that. I sent #5927 as an improved fix. If people are able to take a look, I think it'd be nice to get into 2.7