[esm] should not default to an explicit --conditions=development on development

[JounQin]

Describe the bug

Some packages use exports like the following:

{
  "exports": {
    "development": "./dev/index.js",
    "default": "./index.js"
  }
}

development entry is only intended to be used when using node --conditions=development example.js, all other environment should use the default entry instead.

But for now, vite is using the development entry on vite dev

Reproduction

remarkjs/react-markdown#632 (comment)

See also micromark/micromark#87

System Info

N/A

Used Package Manager

yarn

Logs

Uncaught Error: Module "assert" has been externalized for browser compatibility and cannot be accessed in client code.
at Object.get (browser-external:assert:3)
at go (create-tokenizer.js:213)
at main (create-tokenizer.js:198)
at Object.write (create-tokenizer.js:124)
at fromMarkdown (index.js:117)
at parser (index.js:15)
at Function.parse2 [as parse] (index.js:273)
at ReactMarkdown (react-markdown.js:102)
at renderWithHooks (react-dom.development.js:14985)
at mountIndeterminateComponent (react-dom.development.js:17811)

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.

[wooorm]

I think the problem arises because in Node production/development conditions have to be passed explicitly, resulting in three states in Node: default, development, and production.
Package maintainers can then assume that development code is used when an end-user specifically requests it. Vite always passes one. I think it either shouldn’t pass that condition or handle code found with that condition differently (such as allowing node:assert).

[bluwy]

@JounQin @wooorm I've only noticed this issue now, I'll put this in the team board and discuss soon in the coming meetings.

My two cents on this is that I don't think Vite should be removing the development/production condition. Those two conditions aren't a Node-only feature (source) and can be used agnostically. Things that are Node-only would use the node condition instead. So I think option 3 in micromark/micromark#87 (comment) using nested conditions would be the correct way forward.

Another thing is that in the ESM world there's no process.env.NODE_ENV, so development/production are the only way to define runtime environment-specific code. It won't work if those conditions are Node-only.

[wooorm]

Thanks! To rephrase and summarize: exports from package.json has three conditions: development, production, and default/absence. Vite has two. I believe that development is supposed to be opt-in, which is how Node treats it. Not a default, which is how Vite treats it.

I am not advocating to remove this condition. I agree they are not Node-specific.

I am advocating that, if Vite chooses development, it either a) has to do so because a user explicitly wants it, or b) it should expect instrumented development code to load (such as assert or debug, or just a bunch of code).

[bluwy]

I believe that development is supposed to be opt-in, which is how Node treats it. Not a default, which is how Vite treats it.

Vite has quite some defaults for conditions. I took a look other bundlers, webpack and wmr both support development/production envs by default too. So the ecosystem had kinda moved towards this preference, and I think it's a good thing as you'd always want these optimizations by default.

it should expect instrumented development code to load (such as assert or debug, or just a bunch of code).

I think a bunch of code is fine, but Vite trips up with assert and debug because those work in nodejs only. And because development/production aren't Node-specific, they can be used for browser code as well, which those modules won't work. So ultimately having another set of browser/node conditions would fix this problem, and I believe this is the correct way.

If Vite expects development/production to load instrumented development code like assert or debug, then those conditions would be Node-specific, from my point-of-view.

[wooorm]

👍 on treating conditions as not necessarily Node-specific, but then I would recommend also trying to help maintain them. Currently they’re maintained at Node.js, and they want to share the responsibility. Perhaps Vite can help bring them to a non-Node-only place?

Vite trips up with assert and debug because those work in nodejs only

assert is indeed (debug isn’t), though perhaps Vite can be understanding to it in development. Asserts are a whole thing: https://github.com/unassert-js!

[bluwy]

I would recommend also trying to help maintain them. Currently they’re maintained at Node.js, and they want to share the responsibility. Perhaps Vite can help bring them to a non-Node-only place?

Do you mean officially document it somewhere like a spec? I think that's a good idea, though I'm probably not the person for it (yet) 😅

assert is indeed (debug isn’t), though perhaps Vite can be understanding to it in development. Asserts are a whole thing: https://github.com/unassert-js!

Vite currently shims an empty proxy module that warns about nodejs module usage for dev/prod, which likely brought up the issue in the first place. It might be as far as Vite can go at the moment to not be breaking packages. I just checked micromark and it looks like it's already using a browser-friendly assertion library though, which is great!

Also I stumbled upon this discussion evanw/esbuild#1854, though I still think that it's nice Vite supports it by default as it's more than just a bundler.

[wooorm]

I mean being involved in nodejs/loaders#52 (or for example nodejs/node#40708, nodejs/node#40914). The reason I mention this, is that #4815 (comment) mentioned bringing this issue op to more Vite maintainers. As far as I’m aware, Node.js wants to maintain conditions together with the web (tooling) community. But the web (tooling) community is not helping (yet).

It might be as far as Vite can go at the moment to not be breaking packages

Vite currently throws errors/warnings for packages that use Node.js builtins in development. Vite could, instead of errors/warnings, solve those issues. As they were issues before, it should not break anyone, to fix already broken things?

I just checked micromark and it looks like it's already using a browser-friendly assertion library though, which is great!

Yeah, I mitigated this issue after a bunch of Vite users complained a lot. I would rather have it that I didn’t have to solve problems that only occur in Vite though 😉

Also I stumbled upon this discussion evanw/esbuild#1854, though I still think that it's nice Vite supports it by default as it's more than just a bundler.

Nice, thanks for linking it. Seems the folks there agree with me? (“Every condition requires that the package author opts into it (by defining it) and the consumer of said package declaring that they want to match the condition.”) This is exactly this issue with Vite. The dev/prod conditions are supposed to be opted-into by users.

[bluwy]

Sweet thanks for the links. I'll look into those.

Vite currently throws errors/warnings for packages that use Node.js builtins in development. Vite could, instead of errors/warnings, solve those issues. As they were issues before, it should not break anyone, to fix already broken things?

Vite's philosophy especially with nodejs modules is that they shouldn't be handled/polyfilled by default. In most cases you don't want a polyfill in the final browser bundle as it incurs a huge payload. Webpack 5 had also took this stance, and I think having a distincition of browser/node is great too.

Nice, thanks for linking it. Seems the folks there agree with me? (“Every condition requires that the package author opts into it (by defining it) and the consumer of said package declaring that they want to match the condition.”) This is exactly this issue with Vite. The dev/prod conditions are supposed to be opted-into by users.

I think being "opt-in" has its place too for generic bundlers like rollup and esbuild, as they don't have a sense of dev and build differences (their focus is to only bundle). Vite, webpack, and WMR on the other hand aren't just bundlers, but they apply framework-ish opinions and provide sensible defaults for the end-user, so "opt-in" already turned on by default makes sense to me.

[patak-dev]

We talked with the team today about this issue. Thanks a lot for the discussion @wooorm and @bluwy. Closing it because as it was pointed out by Blu, Vite "opt-in" is by design given the tool's target use cases.