module: development and production exports conditions

[guybedford]

This supports two new conditions in conditional exports - "development" and "production". By default the "development" condition is always matched unless process.env.NODE_ENV is set to "production" in which case the "production" condition is matched.

By making this implementation simply a specification of what the standard in the ecosystem already is, the hope is that this will provide a simple to adopt incremental approach to the problem of production and development condition branching.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[guybedford]

@nodejs/modules-active-members

[jkrems]

LGTM, I think this will be super valuable for packages that want to switch from their previous solution (e.g. conditional require in a proxy file) to a solution that works well with native module graphs. TLA+import() won't properly preserve bindings, so it's not a full alternative to this.

[guybedford]

I've also updated the conditional exports docs example to include this feature in the latest commit, while also demonstrating nesting and fallbacks. We don't currently have enough examples of conditional exports patterns, so hopefully this is somewhat of a first step without polluting the docs.

[ljharb]

i'm not clear on how this works at all. is this production for "require" or for "import"? what about for "browser" or for "node"?

[guybedford]

I just updated the example to a case where production and browser are used together. These conditions are fully orthogonal if we are still allowed to use that word, so by default it applies for both require and import.

[bmeck]

What would be the plan if we were to add additional environment values to the development/production enum? E.G. test. I'm less interested in adding test than how we would deal with NODE_ENV=test changing conditions in a forwards manner (could just call it breaking always 🤷 ).

[guybedford]

What would be the plan if we were to add additional environment values to the development/production enum?

I think we need to look at this on a case-by-case basis. For example, a test runner can already enable a test condition using loaders right now, and a test runner probably already wants to use loaders anyway, so I'm not sure it's a given that Node.js core needs to implement a test condition itself.

Another option would be to support comma-separated conditions in process.env.NODE_ENV. The question then is just if we really do want to expose this for full customization? The benefit of an explicit implementation like this one is the ability to handle mutually exclusive conditions well. I suppose test is an example of one not in need of a default opposite. Perhaps that would work for other user conditions too.

[bmeck]

I'm less interested in adding test than how we would deal with NODE_ENV=test changing conditions in a forwards manner (could just call it breaking always 🤷 ).

I'm not really interested in adding any values. I think development/production are fine for now and likely do not want to ever have a comma separated list personally. I just want to know what would be a plan if we did add one in the future. People using NODE_ENV=foo would be given a development condition in Node version X and in X+1 they do not get a development condition and do get a different foo condition. We could document that we should treat anything that alters environments away from a defaulted development condition as semver major.

[addaleax]

@nodejs/tsc I think you might want to be aware of this, this PR makes NODE_ENV a relevant environment variable for Node.js itself, which is something we’ve declined to do so far.

I tend towards being -1 on this. If we want to modify Node’s behavior, the standard way to do that is through CLI flags, including ones that would end up in NODE_OPTIONS. I feel like starting to support NODE_ENV for some things might be confusing.

Alternatively, could the environment variable name be encoded into the condition in the package.json file itself? I think that would come with the most flexibility for users here.

[bmeck]

I'm a bit wary of encoding things from the environment arbitrarily. We wouldn't want to include things like:

{
  "exports": {
    "USER=root":"badThing",
    "default":"safeThing"}
}

Limiting to a whitelist is a possibility so that only NODE_ENV=... is searched. However, I don't see what we actually gain by the syntax of VAR=VALUE or similar doing so, and the ecosystem convention is to use NODE_ENV with very specific values (see bugs from people using NODE_ENV=prod). I'm also concerned about string encoding if we allow arbitrary values in the condition keys.

[sam-github]

I can see why having exports specific to either browser or node makes sense.

Having them for NODE_ENV seems more like a foot gun than a feature, though. Should NODE_ENV really affect a module's API? Or do I misunderstand this feature?

[bmeck]

@sam-github the intent is more to allow a development implementation which may be slower to allow easier debugging/have side effects for devtools vs a production implementation generally when I've seen this talked about: see https://twitter.com/sebmarkbage/status/1250459342249160710 . I don't think providing alternative APIs is a general goal, but I could see development builds intentionally exposing APIs that a production build does not.

[mcollina]

Unfortunately there is a significant amount of tooling in use at companies that loads configuration values depending on NODE_ENV. It conflicts with this work, because it would prevent them to have a “production” build in a “staging” environment.

I’m -1.

[jkrems]

It conflicts with this work, because it would prevent them to have a “production” build in a “staging” environment.

For tools that use this solution today, isn't that already true? E.g. react will turn on its development messages and non-optimized messages. Without this feature, they are likely to go with something like const impl = await import(process.env.NODE_NODE === 'production' ? './prod.mjs' : './dev.mjs'). So if a package wants to ship optimized prod builds, they won't show up with NODE_ENV=staging either way afaict.

[addaleax]

If this is done in its current form, NODE_ENV needs to be added to lib/internal/main/print_help.js and doc/node.1.

[guybedford]

@addaleax thanks for the clear direction here.

@mcollina if you could get back to @jkrems on his response to your -1 here in #32869 (comment) that would be really great. We will be discussing this at the next modules meeting so it would help to ensure we have worked through the feedback as much as possible. Alternatively if you are able to make the Wednesday meeting to discuss this in more detail that would be a huge help for us to be able to assess directions forward here.

[mcollina]

For tools that use this solution today, isn't that already true? E.g. react will turn on its development messages and non-optimized messages.

I disagree with that choice as well, and I think it sets a bad precedent. However, it is their choice. I've seen so many cases where that implementation causes significant issues to approve this.

My -1 is firm, if this should move forward it can go to a @nodesjs/tsc vote.

[guybedford]

Thanks @mcollina - there's a wide design space here so it's more about trying to understand the root of the concern so that we can inform the design based on this TSC feedback. But it is no use having those conversations without having the full feedback.

To try and capture your hard block succinctly, would you say the following statement covers your concerns: that you are against using the existing process.env.NODE_ENV value because it might conflict with existing uses today in applications.

To dig a little deeper into that:

1. Do you object to this resolver feature entirely?
2. Do you object to any other environment variable approaches?
3. If another new environment variable or flag were used (say for example process.env.NODE_CONDITIONS or --node-conditions=production or something like that), would you object to NODE_ENV=production informing the default value of NODE_CONDITIONS given that another opt-out would exist via the explicit mechanism?

The primary benefit of using this existing approach is that a lot of libraries like React (https://unpkg.com/[email protected]/index.js, and see also the inspiration for this PR in https://twitter.com/sebmarkbage/status/1250453658904326144) use this pattern already, such that it is encoded in many developer build workflows to define this environment variable. If we create a new mechanism the cost to users will be they now have to do two separate things! So we're making more work by not adopting the existing cowpath first and foremost. In particular the answer to (3) would be very useful to understand if there might be a middle ground on the cowpath as it were.

[mcollina]

The key problem I have with this approach is that it makes impossible to configure one library in one way vs another. It's unflexible, uncustomizable and hard to operate in production. Whenever there are incidents, it makes debugging and diagnosing thing harder.

Sanctioning this as the "official and recommended" approach is what troubles me the most. Note that Node.js does not support NODE_ENV=production because it implies that the system is not tuned for production usage by default. Those production-enabling flags create "secret knowledge" on how to operate a Node.js servers in production. This is not the case right now, and I would like to retain this. I'm ok in having an opt-in "debug" mode instead of opt-in "production".

I think having a flag or an env variable is identical for me, as long as it's specific and not overloaded with other meaning, e.g. MODULE_RESOLUTION or --module-resolution (or both) would work.

[guybedford]

I think having a flag or an env variable is identical for me, as long as it's specific and not overloaded with other meaning, e.g. MODULE_RESOLUTION or --module-resolution (or both) would work.

Do you mean specific to production here? So if we had the module-resolution flag that was more generic like --module-resolution=production,any-custom-condition that would avoid the concern? This would behave similarly as we do for these in context.conditions for the resolver hook in loaders (https://nodejs.org/dist/latest-v13.x/docs/api/esm.html#esm_code_resolve_code_hook).

In terms of the specific dev/production feature - yes it is a common deployment problem that development-time features can be kept on, and this feature exactly should be designed with this in mind. If we remove the constraint of following the existing process.env.NODE_ENV pattern, then yes we can certainly switch things around and make dev the opt-in, which in turn should help make users aware of it and that they should disable it, rather than production being invisible as you say.

So with that in mind, would an endorsed --development flag be something you'd consider to mitigate that argument?

Thanks for working through these details, it is very beneficial to see where the arguments land here.

[Andarist]

Just my 2 cents from the both user's and author's perspective. I don't mind this being based NODE_ENV, a separate process flag or anything else. Values for the thing (prod/dev/debug/*) is also irrelevant. I believe that the ecosystem can adapt to a new standard - truth to be told existing NODE_ENV conditions are somewhat quirky and often requires some additional "proxy" files to make library setup "right".

I strongly believe though that there is a big interest from authors' side (which translates directly to users' side as well) to be able to ship 2 versions of their library and have the appropriate one being picked based on a single, standard~, value in the build pipeline/process. I see this PR as a great standardization point because those things are being already spread across the ecosystem, but yet it's all based on some assumption and aggregated knowledge in people's minds. There is no existing good point of reference on how to:

• author library in a way to support 2 modes
• build it correctly (what kind of a consumable output should it have)
• consume the library which uses those patterns.

And I see people tripping over this on a regular basis because of that.

@guybedford thanks for championing this 🙏

[mcollina]

I would prefer to have individual flag first and a “development” mode potentially added later as a preset (a different PR).

We need to consider the fact that a lot of devs change NODE_ENV from within their Node.js application to alter these behavior. Do you think it would be possible somehow?

I agree that having an endorsed way of doing this is important for the ecosystem. It would fix a lot of issues.

[guybedford]

We need to consider the fact that a lot of devs change NODE_ENV from within their Node.js application to alter these behavior. Do you think it would be possible somehow?

This would be possible once we have a VM/Realm API for spawning subloaders (we had this discussion previously). Otherwise it needs to be set in the application loader on init (and the boot loader is becoming a more important concept in Node.js). We likely wouldn't get consensus to have a dynamic aspect in the resolver from within the environment itself, as that breaks the static invariant of resolution. It's important to set it on loader init so that the resolution process itself remains deterministic from within the environment.

I would prefer to have individual flag first and a “development” mode potentially added later as a preset (a different PR).

So we do actually have a way to do this right now with a loader like:

devloader.js

export function resolve (specifier, context, parentResolve) {
  return parentResolve(specifier, { ...context, conditions: [...context.conditions, 'development'] });
}

and running node --loader ./devloader.js app.js.

If we want a generic dedicated flag or environment variable that could be an option, we just need to be sure we want to expose it at this high level and which of those two to choose.

Alternatively, given that the above already provides the means, perhaps we could look at a node --development app.js flag, which both sets an environment variable and provides that condition?

So there seem to be these two main questions:

• Should we focus on the specific development use case or complete generic conditions?
• Should we handle this via a flag or via an environment variable?

I'm still leaning towards the specific development scenario because that solves the problem and provides guidance for the ecosystem, given that the overall use cases are possible with loaders. In that case having a corresponding environment variable seems sensible to me.

I keep trying to come up with examples of other useful conditions to set here, but tend to worry about exposing this stuff too and it does feel like this is one of the most important ones.

[ljharb]

tbh I think it's far too early to be convinced that this is the pattern that should guide the ecosystem.

[mcollina]

This would be possible once we have a VM/Realm API for spawning subloaders (we had this discussion previously).

I think this depends on having that available. I would prefer not to commit to any mechanism that provides a worse user experience that that a dynamic evaluation of NODE_ENV provide currently.

[bmeck]

@mcollina you seem to be saddling a very fine line of not wanting things like NODE_ENV but also wanting the same experience of NODE_ENV. Can you refine what the exact thing you are trying to preserve about dynamically altering NODE_ENV at runtime, that behavior does not mesh with loading in most libraries I've seen that have an if near the top and do not look up the value dynamically after initial startup for example: https://github.com/facebook/react/blob/baff5cc2f69d30589a5dc65b089e47765437294b/packages/react-is/npm/index.js#L3 . Since ESM does not allow running code prior to resolving the graph, if we follow the conventions of the ecosystem this would match the non-dynamic behavior of this PR but if we use a dynamic lookup it would add computational cost at every call site. I'm trying to understand what is desired to be dynamic as performing a runtime mutation and lookup doesn't seem to match performance concerns or ecosystem expectation currently.

[devsnek]

Came here a bit late, but this seems like EXACTLY the thing we have loaders for, especially considering it involves development environments.

[GeoffreyBooth]

Came here a bit late, but this seems like EXACTLY the thing we have loaders for, especially considering it involves development environments.

This was my first thought as well, especially considering that the loader in question is so simple (see Guy's one-liner above). However I think the reason to put this in core is to standardize a pattern for doing this, so that it's not just useful for the user's own code but also the libraries they want to import. If React and Vue and so on all know to publish a development condition, that makes it easier for the user to turn dev mode on system-wide if that's what they want; rather than each library coming up with their own solution (e.g. NODE_ENV, require('lib/dev'), etc.) and the user needing to figure out each one and implement each case in a loader.

[devsnek]

seems like more discussion on what a "development mode" for node would be is needed, how that plays into the wider ecosystem, etc. I think it involves a lot more than just how node resolves files.

[sokra]

My 2 cents regarding interop with webpack:

In general I like this idea.

I would favor an CLI argument --development over using a env variable NODE_ENV. That would prevent people from fiddling with NODE_ENV during the runtime. I think it makes conceptually more sense since NODE_ENV is technically mutable during the runtime of the application, but I don't think resolving should be.

Arguments above about only having a development flag seams reasonable to me and it seems fine to only have a development condition and default can be used to the production case. Users that do not know about this feature would automatically run the fast code. Less chance of accidents and existing workflows can be kept.

In general I'm a fan of custom conditions and in webpack we plan to have that independent of how node decides about custom conditions. I'm fine if node doesn't support it, but also think it would be kind of useful. This could be another discussion after this PR.

As far as I know ESM adoption of react is blocked by this feature.

I don't think await import(process.env.NODE_ENV ? "a" : "b") is a good way to go, as it has additional side-effects due to usage of top-level-await and doesn't enforce to be statically analyse-able.

[guybedford]

I've created a new PR at #33171 implementing the --dev flag approach, pretty much exactly as outlined by @sokra in the previous comment.