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

cli: --dev flag for development exports resolution condition #33171

Closed
wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Apr 30, 2020

This implements a node --dev flag which sets the "development" resolution condition in package exports.

This is based on feedback from the previous PR at #32869, with the following changes:

  • process.env.NODE_ENV is no longer the switch - meaning existing usage of process.env.NODE_ENV will not alter resolution making this backwards compatible and opt-in.
  • The "production" condition is no longer set making the focus on workflows that alter their behaviour in development as opposed to for production, avoiding the problem of users forgetting to set production options.

Node.js supporting "development" in exports resolution will help bundlers and other tools to use this condition as well in their environments. This may well happen regardless of whether Node.js adopts this approach, but having a universal support for the feature would be a huge benefit to the ecosystem as a whole.

@nodejs/modules-active-members

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 30, 2020
@addaleax addaleax added cli Issues and PRs related to the Node.js command line interface. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 30, 2020
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@guybedford guybedford force-pushed the dev-flag branch 2 times, most recently from a517d80 to 9ba5e97 Compare April 30, 2020 19:20
@guybedford guybedford changed the title cli: --dev flag for module resolution and env cli: --dev flag for development exports resolution condition Apr 30, 2020
@guybedford
Copy link
Contributor Author

Ok, I've updated this PR to just reflect the conditional exports resolution, and not add any local environment variable at all. Perhaps in future we could consider something like import.meta.dev even then.

doc/api/esm.md Show resolved Hide resolved
@jasnell
Copy link
Member

jasnell commented Apr 30, 2020

I don't have a better suggestion at the moment, but I'd prefer a more specific, less generic name for the flag. --dev could mean many things and I would fully expect users to confuse it as an alternative for NODE_ENV.

@guybedford
Copy link
Contributor Author

@jasnell I would still like us to eventually handle that environment use case with this same flag - leaving it out for now is more of a compromise for consensus. I'm definitely open to other names but that would be my main concern with something like --development-condition or --development-resolution that is specific to the resolver.

@GeoffreyBooth
Copy link
Member

I think if the condition is "development" then the flag should be --development. It feels unnecessarily confusing (and therefore bad UX) for them to differ.

@devsnek
Copy link
Member

devsnek commented May 1, 2020

Can you elaborate on the design of this flag? (how it affects env, or anything else). This seems like a fairly important addition/change to node's development model flying very low under the radar.

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think the arguments for --development (matching the condition name verbatim) are pretty convincing but it's not a blocker for me at least.

@weswigham
Copy link

Beyond thinking that "a dev mode is important", is there a good reason this shouldn't just be a flag that activates an arbitrary list of resolver conditions? Eg, --cond dev,nomin,whatever? Is there really a need to be prescriptive in core here?

@weswigham
Copy link

Oh, also of note, from a less technical but more philosophical perspective: conditions like dev vs prod mode make static analysis a pain, so, professionally, I can only hope this is actually never used. Sure, it is easier to resolve a graph under a resolver condition set than an environment variable (kinda), but the expectation then becomes that we resolve and reconcile all possible graphs (somehow). This is essentially like #define's in c templates, which make for really, really terrible IDE experiences. (You pick a set to activate, and the editor pretends the rest don't exist, resulting in unchecked code.) It's arguably better that the dev vs prod mode swap is dynamic, since dynamic code like that is pessimistically type checked for compatability; while various possible module graphs under some arbitrary conditions are... Not.

@guybedford
Copy link
Contributor Author

Beyond thinking that "a dev mode is important", is there a good reason this shouldn't just be a flag that activates an arbitrary list of resolver conditions? Eg, --cond dev,nomin,whatever? Is there really a need to be prescriptive in core here?

The exact problem being solved by this PR is that excluding dev imports requires a TLA pattern like Tobias mentioned at the end of his comment in #32869 (comment). The solution is very much driven by direct library and bundler needs - which is exactly about static optimization. This is about improving static analysis, and I use these patterns a lot in my own static analysis tooling.

@weswigham
Copy link

The exact problem being solved by this PR is that excluding dev imports requires a TLA pattern like Tobias mentioned at the end of his comment

Which is already treeshakable in build tools with constant folding, and is pessimistically type checkable, using only constructs that explicitly appear in the code. The problem with removing the branching location from the code itself (and make no mistake, this is just moving a branch from within the code to within the runtime), is that analysis of that branch must also become implicit, and no static analysis tool is currently set up to do that. This is breaking all existing tooling in the service of making future tooling "simpler". IMO, that's not a great look.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i forgot to hit the red button on my earlier comment

@Andarist
Copy link
Contributor

Andarist commented May 1, 2020

@weswigham the problem with TLA approach is also that it's hard to setup right for a library that wants to provide both dev and prod versions - it's not impossible, but requires more static analysis to be done so the correct "TLA entry" can be created as you need to conditionally generate default export and all export names (you can't reexport from awaited module, right?).

It also has a caveat of prod/dev files not being easily discoverable through package.json metadata. People are often confused (IMHO rightfully so) that process.env.NODE_ENV reference is left in the consumable code which cant be consumed directly in a browser as there is no process in that environment. Standardizing this through package.json metadata has the advantage of both dev and prod files being directly usable in browsers (if the particular package is intended to be browser-compatible ofc).

@guybedford
Copy link
Contributor Author

@devsnek has the discussion here answered your question? If not, can you perhaps clarify your concerns further?

@devsnek
Copy link
Member

devsnek commented May 4, 2020

@guybedford currently there is no such concept as "development" or "production" mode in node, it's just in a curated balance between verbose and fast all of the time. if we're adding a dev mode, i think there needs to be more information about when someone should use it, what kind of changes they should expect from node when it is enabled or disabled, policy on how we use that development mode within core (extra checks in internal code, extra debug stuff enabled?), process.mode or something similar, more documentation around idiomatic usage of node in production and development mods, etc...

alternatively (my preference), this flag can be renamed to --loader-mode or something, and it should probably leave the mode names to userland, the same way we do for NODE_ENV.

@guybedford
Copy link
Contributor Author

@devsnek definitely agreed a development mode should not be taken lightly but it is very much a clear need as a feature for Node.js that is covered by the process.env.NODE_ENV pattern today that we can possibly improve on.

I would like to see a variable that has access to this mode as this PR first included, the hard part is just we can't get consensus on that right now and don't know how to do that in a universal way without globals either.

If we want to block this feature on having a variable accessible I'm happy to discuss that, but my hope was we could consider this to be a gradually developed feature over time.

A more generic resolver-only condition-setting flag as you say is certainly an alternative if others turn out to be against this approach, but I'd still prefer an overall dev mode if others can support it as a concept.

@GeoffreyBooth GeoffreyBooth added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label May 4, 2020
@devsnek
Copy link
Member

devsnek commented May 4, 2020

@guybedford personally i'd like to at least see the runtime variable exposed as part of landing a development mode. i would imagine others have their own interests. @mcollina ccing you here since you had opinions in the other thread

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a way to check if this is turned on from code.

Good work anyway, this is in line with what I would have imagined.

@bmeck
Copy link
Member

bmeck commented May 5, 2020

Can we expose the static conditions entirely process.dedaultConditions or are you just asking for mode ala process.mode or some such?

@jasnell
Copy link
Member

jasnell commented May 6, 2020

-1 on adding anything to process, but dropping a property off util would be good I think. Alternatively, exposing the internal options check may work.

@jasnell
Copy link
Member

jasnell commented May 6, 2020

Alternatively, we could leave it up to userland to write up a module to detect dev mode by inspecting process.execArgv and process.env.NODE_OPTIONS. Doing so would not be difficult and does not require any new API surface.

For instance, node --dev -pe "process.execArgv.indexOf('--dev') > -1"

@devsnek
Copy link
Member

devsnek commented May 6, 2020

That makes it seem like we don't want people to use it.

@jkrems
Copy link
Contributor

jkrems commented May 6, 2020

-1 on adding anything to process, but dropping a property off util would be good I think.

One argument for a property on process is that it's globally reachable. That's a good thing because it makes it easier to use in code that may be targeting both node and browsers. In most of today's browser-targeting tools it's easier to do a targeted polyfill or replacement of a global than of an imported module like util or module (without accidentally pulling in too much code). Tools for replacing global expressions are already in place for process.env. Putting it in one of node's modules would require starting from scratch on that front.

We could leave it up to userland to write up a module to detect dev mode by inspecting

I think that direction is generally viable. It may be nice to officially bless one module here though. But it feels like it may be a 1-line module that will end up in 2-3 versions in each app's dependency tree. And even then (see section above), it would be really hard to use it effectively when targeting browsers. It would require reliably inlining something exported from that ecosystem module to trigger dead code elimination.

@jasnell
Copy link
Member

jasnell commented May 6, 2020

If something does get added to process, please make it generalized and not specific to just --dev ... for instance, we can expose something like our internal options check... e.g. process.getOption('--dev'), which can be used for other checks process.getOption('--no-warnings'), etc

@guybedford guybedford removed the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label May 6, 2020
@sokra
Copy link
Contributor

sokra commented May 6, 2020

I want to note that process should not be available in browser-targeted code.

A userland package for that could be implemented using the exports field:

"exports": {
  "development": "./true.json",
  "default": "./false.json"
}

That would be node and browser compatible, afaik using json makes it commonjs and esm compatible.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@sokra
Copy link
Contributor

sokra commented Jun 9, 2020

What's the state of that?

If node chooses to not implement that and this doesn't become a universal pattern, we have to support both production and development conditions to make support detectable...

@guybedford
Copy link
Contributor Author

This PR has been superseded by #34637.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.