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 42
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 #484 from nodejs/notes-2020-02-12
Add meeting notes for 2020-02-12
- Loading branch information
Showing
1 changed file
with
102 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,102 @@ | ||
| # Node.js Foundation Modules Team Meeting 2020-02-12 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: http://www.youtube.com/watch?v=n9CpwXe5nHE | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/483 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/1HgbR0o38ALVCxAoNdv2IRpIfTgxP_X1n3lwCBdEBupI/edit | ||
|
|
||
| ## Present | ||
|
|
||
| * Modules team: @nodejs/modules | ||
| * Jan Krems (@jkrems) | ||
| * Saleh Abdel Motaal (@SMotaal) | ||
| * Wesley Wigham (@weswigham) | ||
| * Geoffrey Booth (@GeoffreyBooth) | ||
| * Corey Farrell (@coreyfarrell) | ||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| *Extracted from **modules-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. | ||
|
|
||
| ### nodejs/node | ||
|
|
||
| #### Self ref bug by Jordan | ||
|
|
||
| * self import works by accident in 12.16 without flag, need fix PR | ||
| * Guy is working on a fix | ||
|
|
||
| #### module: disable exports encapsulation for CJS [#31718] | ||
|
|
||
|
|
||
| * [GB] Based on an incident with Preact. They enabled exports for conditional branching but they broke existing (CJS) consumers so they disabled encapsulation via ./ export. | ||
| * [GB] Right now opt-out is for both require and import. Risk is that all people will disable because of CJS consumers. Opt-out is `"./": { "require": "./" }`. | ||
| * [MB] Document opt-out and make sure we warn about downsides of opt-out. | ||
| * [WW] Adding explicit exports should be documented as being preferred. | ||
| * [JK] Medium-term tools may add exports support and only opting out for require will stop being a solution. It will bring existing ESM consumers that will hit the whitelisting. | ||
| * [CF] Conditional opt-out as above will trigger the experimental warning. | ||
| * [GB] Should we remove the conditional exports warning? | ||
| * [MB] I’m fine with warning, not a huge problem in practice. Unconditional opt-out is likely preferred. Conditional opt-out may be confusing to consumers | ||
| * [WW] People should accept that exports encapsulation is potentially breaking | ||
| * [GB] What Preact ships is conditional exports for import vs require which may trigger warnings | ||
| * [MB] We should encourage them to switch to default. | ||
| * [GB] What do people think about removing the warning? | ||
| * [JK] It feels weird to ask people to work around the warning by choosing a very specific subset of possible conditional exports forms that happens to trick the warning code. | ||
| * [GBB] Could we make the warning depend on published vs not? | ||
| * [MB] Issuing the warning to authors only may be possible. We could set up a timing to remove or reduce both flags and warnings. For example in a month or two. | ||
| * [GB] Will open issue to discuss handling of warnings going forward. | ||
|
|
||
| #### module: Refine and unify exports resolution error handling [#31625](https://github.com/nodejs/node/pull/31625) | ||
|
|
||
| * [GB] Main problem tackled here: Conditional fallback behavior only applies to generally valid values, not for “obviously” bad configuration like invalid URLs. | ||
| * [WW] From a ideal/purity perspective: Does path not exported leak the existence of a guarded entry? | ||
| * [GB] Both not configured at all and configured but conditional not matched is the same error (path not exported). Does not leak file system existence for example. Any thoughts on splitting `MODULE_NOT_FOUND` based on likely cause? | ||
| * [WW] Would prefer not to expose error codes but node makes that impossible. | ||
| * [MB] Error messages are flexible. Important that shared error codes are semantically close enough. | ||
| * [GB] We could try to keep `MODULE_NOT_FOUND` but we already have other forked error codes. | ||
| * [WW] Do we expect people to switch on it? | ||
| * [GB] In node generally, the error code and message go together. We could use module not found and split with different messages. | ||
| * [WW] Mostly a matter of intent: Do we want people to switch on error codes? | ||
| * [JK] People are already catching module not found, it’s a feature that existing catch blocks don’t catch these new conditions. | ||
| * [GBB] Mostly care that error messages are helpful. | ||
| * [GB] Agreed that optional dependency catch blocks shouldn’t accidentally catch encapsulation errors. A lot of this is also based on use in `import.meta.resolve` where having a more fine-grained feedback channel can be valuable. | ||
| * [GB] Any objections? Would love more feedback. | ||
| * *No objections.* | ||
|
|
||
| ### nodejs/modules | ||
|
|
||
| #### Docs/review of various affects of type field [#475](https://github.com/nodejs/modules/issues/475) | ||
|
|
||
| * [MB] Removing from agenda, adding help wanted label | ||
|
|
||
| #### unflagging in 12.x LTS [#450](https://github.com/nodejs/modules/issues/450) | ||
|
|
||
| * [MB] Dropping from agenda | ||
| * [GBB] How do warnings work in 14 around that time? | ||
| * [MB] Separate topic, 14 may be a good time to do it. | ||
| * [GBB] Doesn’t have to correlate with 14.0.0? | ||
| * [MB] Correct. | ||
|
|
||
| #### Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) | ||
|
|
||
| * [MB] Fine without doing it. Have a majority in place if we want to ask for chartering. | ||
| * [GBB] Maybe just move forward with this? Most things are unflagged. | ||
| * [MB] We can make a request and unless somebody objects in a week, we can ask the TSC. Any objections? | ||
| * *No objection* | ||
|
|
||
| #### Loader Hooks [#351](https://github.com/nodejs/modules/issues/351) | ||
|
|
||
| * [GBB] There were ideas for a redesign by Guy and Bradley but not movement yet. One open question was chaining loaders. If we want dramatic changes, we should do it soon. | ||
| * [GB] Worth considering if the API that we want is breaking. If somebody wants to drive, it could be done. Should go back to use cases and look at requirements. Do we know what the status is? | ||
| * [GBB] Move off-thread was another big issue. WASM and extension-less files are hard to support with current order. | ||
| * [JK] HTTP loaders are another example where combining format+source is very useful. | ||
| * [GBB] Should start with use cases and make sure our APIs support them properly. | ||
| * [JK] Likely need a new hook for global init that runs code on the main thread. | ||
|
|
||
| #### Patch/Instrument a module [#339](https://github.com/nodejs/modules/issues/339) | ||
|
|
||
| * [MB] Should we remove this and roll this into loader hooks? | ||
| * [WW] Would like to keep, it seems distinct enough. | ||
| * [GBB] Need to do research on how to change the active code of modules. Maybe something in the debug protocol? |