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 43
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refs: #489 Refs: https://docs.google.com/document/d/1Evh1h_SAq7VnfXJlt5V8-HX5an6JstVdRE9IkowxU5E/edit Closes: #489
- Loading branch information
Showing
1 changed file
with
127 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,127 @@ | ||
| # Node.js Modules Team Meeting 2020-02-26 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: http://www.youtube.com/watch?v=4d0g40sSV-g | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/489 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/1Evh1h_SAq7VnfXJlt5V8-HX5an6JstVdRE9IkowxU5E/edit | ||
|
|
||
| ## Present | ||
|
|
||
| - Myles Borins (@MylesBorins) | ||
| - Bradley Farias (@bmeck) | ||
| - Corey Farrell (@coreyfarrell) | ||
| - Geoffrey Booth (@GeoffreyBooth) | ||
| - Guy Bedford (@guybedford) | ||
| - Jan Krems (@jkrems) | ||
| - Jordan Harband (@ljharb) | ||
| - Rob Palmer (@robpalme) | ||
| - Saleh Abdel Motaal (@SMotaal) | ||
| - Wes Todd (@wesleytodd) | ||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| Extracted from **modules-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. | ||
|
|
||
| ## nodejs/node | ||
|
|
||
| ### module: disable conditional exports, self resolve warnings [#31845](https://github.com/nodejs/node/pull/31845) | ||
|
|
||
| - GB: We had issues with project like Preact that use `exports`. Node 13 users are getting warnings. An ES module lexer was asking the main to point to the ES module file - Guy corrected them to use `module`. They used `exports` and people did not like the warnings. Snowpack was unable to use it until the warnings are gone. If we want to backport to Node 12 in April, we need real world usage feedback. We will not get it. Packages are resisting. We should remove `experimental modules` warnings as well. Let's discuss timelines. When are people comfortable removing them? | ||
| - MB: The balance is ensuring folks using this are aware it's experimental. Sometimes people jump into issues complaining about breaking changes, and we point to the warnings as justification. I don't think we are making big changes to conditional exports. It is conditional exports and self-resolve we are thinking of removing warnings for - not the warning for ESM as a whole. I am comfortable removing those warnings on Node 13. Less clear on 12. So let's remove on 13, wait a bit before backporint. | ||
| - GBB: On 12, they wouldn't see the warning unless they opt in by the flag. | ||
| - JH: There was a brief bug on 12.6 that is fixed now. | ||
| - MB: It's good to keep consistency between 12 and 13. | ||
| - JK: +1 on removing warnings. The effect was supposed to be to advertise its experimental. In practice, people find magical ways to avoid triggering the warning. They use the feature but actively hide the warning. If removing the warning matches the world... it is what is happening. People are using the feature. Right now it's painful they have to use a specific way to avoid the warning. | ||
| - MB: The warning achieved part of its goal - people got annoyed and told us, which told us people were adopting it. The risk is that we will stop receiving feedback. | ||
| - JK: I raised this because I found an issue on a smaller project that was copying a bigger project. | ||
| - MB: They are writing things such that in CJS they are not using experimental things. I am being pedantic. I think we should remove the warning. | ||
| - JH: The point of the warning is to let people know. The author is doing this. The user is not. The warning is mis-targeted. We could make exports a non-breaking change. Preact 3.1 is breaking changes vs 3.0. Despite our warnings, people use this feature in minor bumps. We here about it from consumers. JK and GBB have a point - users know how to subvert the warning so it's not achieving much. Features like this leak. They are not encapsulated. At this point, things are using them. The ability to break them is kinda irrelevant. So let's remove the warnings. | ||
| - MB: The warnings about unsafe buffers was more scoped but is more effort. Is it worth it? | ||
| - JH: Unless they are using self reference, they are not using the feature in their own code, so their own tests won't trigger the warning. So a targeted warning would not help. | ||
| - MB: Let's get consensus. Any objection to removing warnings on 13 for conditional exports and self-referencing? | ||
| - *silence* => consensus is reached | ||
| - GB: I will update the PR to only remove these warnings. | ||
| - MB: Start a new issue thread for removing the overall ES modules warning. I don't agree with removing it yet. | ||
| - JK: Having a PR open for it would be good. Not many weeks left until we can remove the warnings in Node 13. | ||
| - MB: I do not think we will remove the warning in April when we unflag in Node 12. | ||
| - GB: This is news. | ||
| - JH: Talk about it next meeting? | ||
| - GBB: Are we going to backport the conditional exports and self-references warning removal to Node 12? | ||
| - MB: That's a separate issue from what we discussed today. | ||
| - JK: No one is accidentally triggering this on 12 because it's already behind a flag. | ||
| - GBB: That's why I thought it would be uncontroversial. I just want them to be similar. | ||
| - MB: Agree on principle. On warnings, and stability index, it's different for LTS branches. | ||
| - GBB: Ok. We can make a separate issue. | ||
|
|
||
|
|
||
| ### Timeline Discussion | ||
|
|
||
| - MB: I think removing warning for experimental modules is not something we should do until we are close to stabilising the API. I was unsure if we wanted to remove it when 14 is being cut. We probably want it gone by 14 LTS. I want to see more usage of the feature without the flag first. fs.promises had a warning for *much* longer. We have a history of warnings on other APIs. There is a history of removing them during LTS. May even be premature to remove in 13. That is where I am coming from. | ||
| - GB: We have to think about the utility of the warnings. It discourages adoption. Users do not like warnings. They are part of the UI of Node. Users cannot control it. We're discussing backporting to 12 LTS. If we want to discourage adoption, then I don't see how backporting is not as important as removing the warning. This process of getting modules into Node.js has taken a long time. Tendency towards taking longer is not good. We want to prove we can move forward. | ||
| - JK: The impact. Removing would be that if I want to use modules in a CLI tool I publish to npm, I cannot publish until the warnings are removed. There are good reasons to remove the warning sooner rather than later. The process has been long. We would lose momentum by not unflagging. | ||
| - MB: Slowing down adoption is potentially a feature not a bug. If we unflag on LTS we will get an exponential curve of more adoption, bugs, usage, issues. If we got something wrong, we have far less room to fix it. We're talking about months not years of difference. These features have only been worked on for less than a year. They are not as mature as we would like them to be. Node 10 is still maintained so people can't ship things until next April so folks can ship modules without breaking other folks. If we sketch out timelines, worst vs best, when does Node 10 go out of scope. I will create an issue and raise this in the next meeting. | ||
|
|
||
|
|
||
| ### Add support for extensionless workers and ESM eval [#31760](https://github.com/nodejs/node/pull/31760) | ||
|
|
||
| - MB: Upstream PR. The work is happening. | ||
| - BF: We need to be aware of it. The PR goes against one of the design patterns we use. They are having the consumer define the format of the target resource. The thread has alternatives. Nothing needs to happen here. Please look at the issue. | ||
| - MB: I will remove "Modules agenda" label from it. If you see stuff, label it. | ||
|
|
||
|
|
||
| ### WIP: Move ESM loaders to worker thread [#31229](https://github.com/nodejs/node/pull/31229) | ||
|
|
||
| - BF: PR has been open for a while. Global attentuation. We have to split the work. Move forward or block it. We've tried past approaches. One was adding a hook for preloads. Was very hard to wrangle. then I tried allowing loader hooks to give us source-text to be run on bootstrap - but async work is very tough because of how async hooks work. Could be synchronous if we run in the script goal. Then we could figure out interleaving later. You could "use strict" in there yourself. You need access to `require`. This is not for APMs. | ||
| - GB: Falls under the same bracket. | ||
| - BF: People are using dynamicInstantiate to punch globals around. Not to mutate existing modules. | ||
| - GB: Also, a loader should be able to be self-contained. Use of loader should not invoke a require flag. APMs need to hook in. | ||
| - BF: Separate feature. | ||
| - GB: This is critical. It makes loaders less useful. | ||
| - BF: It's not supported yet. | ||
| - GB: But today's it's possible. Loaders can hook into built-ins. | ||
| - BF: They can do it via bootstrapping. If that's the requirement, this PR will remain blocked for a long time. | ||
| - GB: As long as attenuation has access to require. | ||
| - JK: Agree with Guy. Some way of running code globally before app code runs ought to be able to load builtin modules. That captures many use-cases. Could use --require. One with bootstrap. One with loader hooks. That also applies to any attenuation code, so this is not a critical argument. We need to decide. Block this PR on bootstrap/attentuation code on stuff that could be passed into require, or can we move forward even though loaders cannot do attentuation and need a second flag. | ||
| - BF: We're not saying it's like that forever, even if we go forward. | ||
|
|
||
|
|
||
| ## nodejs/modules | ||
|
|
||
| ### Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) | ||
|
|
||
|
|
||
| - MB: Next step is a PR to the tsc repo. A doc is there full of all the working groups, including a scope statement. Streams scope seems similar to what we should be asking for - it says "we will take responsibility" rather than asking for sign-off on every change. | ||
| - JH: It would be nice to be chartered. Recognition is one part. Seems like the right thing to do. More decisions will be needed in future. | ||
| - MB: TSC can decharter if the group disbanded. | ||
| - GBB: The issue about wasm modules. We've expanding package.json. There may be lots of groups wanting to drive things into that crack. So maybe it's worth being chartered. | ||
| - MB: Stream scope is really good. Specifically, nothing here says they own all decisions. I would be uncomfortable saying we have total ownership of part of a codebase. | ||
| - WT: With package maintenance group, we added to package.json. npm just added it without process. Whilst I think it's good to have a group that is not npm, it should not matter for other groups. | ||
| - MB: Does anyone want to take the time to draft a charter? | ||
| - GBB: Open to doing it. Not sure when. | ||
| - JH: Same. | ||
| - MB: Two options. Keep it open? Any objections to chartering? Any objections to closing the issue? | ||
| - JH: Keep it open. | ||
| - MB: If no one has time to do the work, it's a relatively strong signal that we're not being blocked by our status either way. Maybe at some point we just close the issue. One advantage is discoverability. | ||
|
|
||
|
|
||
| ### Loader Hooks [#351](https://github.com/nodejs/modules/issues/351) | ||
|
|
||
| - GBB: We were supposed to untag it? | ||
| - MB: Objections to unlabelling? | ||
| - *no objections* | ||
|
|
||
|
|
||
| ### Patch/Instrument a module [#339](https://github.com/nodejs/modules/issues/339) | ||
|
|
||
| - GBB: This is the last missing piece. Nothing has happened over the last two weeks. | ||
| - MB: Keep the label? Objections to removing? | ||
| - CF: For NYC, current hooks don't allow multiple hooks/transforms. Suggestion was userspace composition. I don't know how it would work. There could be only one. That's my concern. | ||
| - GBB: The reason the API passes a callback at the end was to make them composable. It might still happen. Someone just needs to do the work. Jan was skeptical about making it work in a reasonable way. The user would have to provide the ordering. Rather than a strange config file, this let's the user compose the loaders. Code over config. Can be done now. That might be the best we can do. | ||
| - CF: My expectation was by order of the arguments. So --loader three times creates a stack. For NYC, it injects a transform into child process. I don't see how we deal with someone using NYC and ts-node or babel. | ||
| - GBB: Please open an issue with the specific issue. | ||
| - CF: ok | ||
| - JK: I was more bullish on multiple loaders before I tried to write them and combine them in a practical way. It's easy to write a loader in a way that is incompatible with other loaders. For example, ts loader checks for file extension. Then I have an HTTP loader that may not even have an extension. The orchestration of multiple loaders is non-trivial. I haven't seen a good story of how the user experience could be good, and the developer experience of writing loaders could be good. It's better to have "the webpack of loader orchestration" for this use-case. You need something smart - Node is not the place to do this - it does not load complex bootstrap files. It's not a good fit. | ||
|
|