This repository has been archived by the owner on Sep 2, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 44
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #506 from nodejs/robpalme-patch-3
doc: Meeting notes 2020-04-22
- Loading branch information
Showing
1 changed file
with
124 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| # Node.js Modules Team Meeting 2020-04-22 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: http://www.youtube.com/watch?v=pYg1thE_Nz0 | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/504 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/1G1coLWXsCPPWsfkgbYR0aXvmSRMoaJIz-ndpVk_2Nlc/edit | ||
|
|
||
| ## Present | ||
|
|
||
| - Myles Borins (@MylesBorins) | ||
| - Jordan Harband (@ljharb) | ||
| - Corey Farrell (@coreyfarrell) | ||
| - Michael Zasso (@targos) | ||
| - Rob Palmer (@robpalme) | ||
| - Bradley Farias (@bmeck) | ||
| - Wesley Wigham (@weswigham) | ||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| - MB: Warning removed on Node 14. Released yesterday. | ||
|
|
||
| ### nodejs/node | ||
|
|
||
| #### module: development and production exports conditions [#32869](https://github.com/nodejs/node/pull/32869) | ||
|
|
||
| - MB: React community inspired this conditional exports. Lots of discussion. Some -1s - mostly about NODE_ENV not normally relied on for Node behaviour. Those env vars don't have clear defaults. Discussed with Matteo. Might change the mental model, e.g. a new env var that is not NODE_ENV. We chose new ecosystem patterns for "type": "module". There was interest in introducing a DEBUG mode to Node - relating to log levels. | ||
| - JHD: Conceptually this is wrong. Resolution should not be affected by environment. Conditional exports is about which implementation do I want. Production and development React have never been different implementations - it's simply warning code deleted in production. Doesn't feel appropriate to moosh this into conditional exports. Not the proper place for environment switching. | ||
| - BF: For React, they are not changing names or signatures. But they do hook into DevTools and run side-effects that should not leak into production. | ||
| - JHD: Some people use those things in production. | ||
| - BF: You should never do that. | ||
| - JHD: Some developers need DevTools in production. | ||
| - BF: We do a network override instead. It's bad. There is an obvious reason to add this. The idea the environment affects the code is beneficial. | ||
| - JHD: I am specifically talking about resolution. | ||
| - BF: But the resolution determines what code is loaded. | ||
| - MB: Some premises the PR is built upon. Independent of the use-case, one thing that came up with Matteo on debuggability was the ability for this to be dynamic over the lifetime of an application. Are conditional exports set in stone at app launch - or change later? NODE_ENV can be changed over the course of the app. 2. Do we want the conditions to be static or incorporate statements? Conditions on the left could be NODE_ENV==production. | ||
| - BF: We have lots of flags for resolution. Symlinks. Home directory for CJS. These are set when Node starts up. We shouldn't make this feature the only dynamic feature. | ||
| - MB: Agree. Dynamicism would re-open closed topics. | ||
| - CF: If people published packages on development, this could surprise people using it on their package, e.g. NODE_DEBUG==my-package. I agree NODE_ENV is a bad idea here. | ||
| - BF: So instead of it being applied to the whole app, you want to apply to specific packages only - not global? | ||
| - CF: Yes - so I can opt in selected packages into debug mode. | ||
| - BF: Might cause issues. Packages can share names. We have well-defined package boundaries now, so it's possible, but may not be realistic. You start to need to know internal package structure of your whole app. | ||
| - MB: Gus brought up that loaders could help with. If ecosystem aligns on patterns of tooling on top of Node. Debug loader could know this stuff. So ecosystem could solve this. We have given people most of the tools they need. | ||
| - BF: I agree that loaders are a power tool that everything can be shoved into. But we don't necessarily need to use them for everything. We have symlink flags - this affects the default loader but custom loaders may not respect this. So it depends whether this is configurable in our loader, or whether ecosystem can do this on their own. Personally I don't think why there is much to discuss about conditions. This is basically a bitfield turned on and off by name. Maybe we should figure out why configuring condition exports is controversial. | ||
| - MB: This condition is controversial because NODE_ENV respect of production/development is not consistent across the ecosystem. | ||
| - BF: I don't know why we don't want this to be configurable. Normally that's for things that would cause security issues. It's hard to configure a loader correctly - much more dangerous. We have object identity going on in the conditions. Seems weird to shove this under loaders. I don't see why configuring this is problematic. NODE_ENV could have collisions or misuse, but if we solve that I don't see the objection. | ||
| - MB: Matteo wanted it to be dynamic. People not thrilled about production vs development. Dynamic vs not dynamic. | ||
| - WW: Not a good idea because a conditionally different API is a bad idea. Makes static analysis harder. | ||
| - BF: Tools already need to deal with process.ENV. Most bundlers support it because it's the standard way. Constant substitution. Would be easier to analyze pkg.json than parsing JS. | ||
| - MB: Next steps are likely: further scope low level assumptions. Then define the feature more. | ||
| - BF: There's not a clear path forwards to me. Because there's no reason to push it either way. | ||
| - MB: Feature request came from the ecosystem. So define a path to concensus or close it. Does it need more discussion? | ||
| - BF: Currently discussion doesn't seem to have direction. Don't close it out. Let's block it on use-case constraints being unclear. | ||
|
|
||
| #### module: empty fallback and null exports as not exported [#32838](https://github.com/nodejs/node/pull/32838) | ||
|
|
||
| - MB: PR from Guy. Limits throwing errors when you pass an empty array or null when you pass it on the right hand side of the exports. | ||
| - BF: Just do it. Sounds fine. | ||
|
|
||
|
|
||
| #### esm: allow configuring default package type [#32394](https://github.com/nodejs/node/pull/32394) | ||
|
|
||
| - BF: This is a build option for people who want to make custom builds of Node. Some people want to change the default package type now or in the future. I made a PR to allow them to do this, but virtually nothing runs if you set it. So it's a playground. I firmly believe this should not be runtime configurable yet, because people will start to rely on it. | ||
| - WW: You can simulate this with a fork of npm/yarn. | ||
| - BF: Parse goal determination is affected in a non-trivial way by changing the package type. There's a package out there that had to be modified because the change in default was weird. This PR let's people test it without opening issues. | ||
| - MB: We debated whether it should use a new binary name, e.g. `node-esm`. Not a hill to die on. | ||
| - JHB: Unclear why this is needs to be made easy. | ||
| - MB: It helps shutdown the debate. | ||
| - JHB: I don't want it to be easy. | ||
| - BF: it's not easy - very few people build Node. | ||
| - JHB: It's just a matter of changing a string in a few places? Like in your PR? | ||
| - BF: There's indirection for configurability but there is logic too. We do some defaulting if the package type does not exist in multiple places. | ||
| - JHB: We could refactor to have a single source of truth. Wes pointed out people can already do this. People can float you closed PR. I question the utility of landing it. | ||
| - BF: People are asking for it so I made a PR to give it to them. Why prevent them from having it? | ||
| - JHB: It causes ecosystem issues if it spreads. | ||
| - BF: It's not easy to build Node. | ||
| - WW: It's dead code. | ||
| - JHB: It's not a thing we want to exist. | ||
| - BF: I want to give people what they want. We can make a recommendation to float this PR. | ||
| - JHB: Then let's do that. | ||
| - BF: That sounds impolite compared to just merging it. | ||
| - MB: Floating patches can go out of date and will not be tested. If we have a test... I don't know if we want to maintain this. It would require a whole other build. So it is a risk landing it. | ||
| - CF: No objections to a build time flag. Runtime flag would induce requests to package owners. | ||
| - MB: We already support `type: commonjs`. Default is an internal setting which could change. We should introduce this if we would ever want to do. | ||
| - JHB: Not sure it would get consensus. | ||
| - MB: Don't see how we could ever change the default. Not trying to bring stop energy. | ||
| - BF: Fine with either way. Could we get consensus to say we will never do this? | ||
| - RP: Feels like a big commitment. | ||
| - MB: We are definitely not going to change it in the next three years. I don't see how we could change it without such big changes that the type field itself would be removed. | ||
| - RP: Seems reasonable. | ||
| - MB: So maybe this does not need to exist, because we might not even need the mechanism. | ||
| - BF: Because 14 defaults to "commonjs", likely all LTS will support .js as being commonjs. Seems like this is a no go to change. Therefore this PR should never exist. | ||
| - JHD: That's my position. | ||
| - BF: I can close it out. | ||
| - WW: CJS is first-class forever. | ||
| - JHD: Top-level await could be an exception. | ||
| - MB: And import.meta. TLA in CJS breaks the world. | ||
| - BF: Don't agree this means CJS is first-class forever. We could allow CJS to be disabled. | ||
| - RP: This feels like we've made a leap here from saying we can't change the default to saying CJS is firs-tclass forever. | ||
| - BF: Agreed. We can close the PR and say we have no intent to change the default in the foreseeable future. | ||
| - JHD: Even folks uncomfortable committing to not changing the defaults forever would agree it won't change in the near term. | ||
|
|
||
|
|
||
| #### Self-reference Sigil | ||
|
|
||
| - JHD: Self-reference independent of package name. If we got consensus, could we add it in semver minor? | ||
| - BF: Sounds ok. | ||
|
|
||
|
|
||
| #### Loaders into worker threads | ||
|
|
||
| - BF: I didn't want it to land earlier because it disables a loader hook. I plan to land it - but it cannot be backported. The dynamicInstantiate would be removed. Any objections? We had an objection to breaking that API - which is why we added a hook to instrument the global env. I'd like to land in a Stable (non-LTS) branch for people to play with | ||
| - CF: In Node 14, not landing the PR, means it can't have non-experimental loaders. | ||
| - BF: Some loaders do use this hook. If we want to backport and we are ok with breaking, we can discuss it. But we haven't got field testing. I don't want to break people until we have had more testing. Then we could backport it. Even though it's experimental we still don't want to break stuff. | ||
|
|
||
| --- | ||
|
|
||
| **END OF MEETING** | ||
|
|
||
| ### nodejs/modules | ||
|
|
||
| * Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) |