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 #456 from nodejs/robpalme-patch-1
docs: notes for 2019-12-18
- Loading branch information
Showing
1 changed file
with
116 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,116 @@ | ||
| # Node.js Foundation Modules Team Meeting 2019-12-18 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: http://www.youtube.com/watch?v=3g5Og5w1iV0 | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/455 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/107-EWDmaUN5XCHNzme6h5AYfl1L3J-ZbgZSq7bqWC8E/edit | ||
|
|
||
| ## Present | ||
|
|
||
| * Myles Borins (@MylesBorins) | ||
| * Jan Krems (@jkrems) | ||
| * Bradley Farias (@bmeck) | ||
| * Rob Palmer (@robpalme) | ||
| * Guy Bedford (@guybedford) | ||
| * Saleh Abdel Motaal (@SMotaal) | ||
| * Wes Wigham | ||
| * Geoffrey Booth (@GeoffreyBooth) | ||
| * Corey Farrell | ||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| *Extracted from **modules-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. | ||
|
|
||
| ### Conditional Exports | ||
|
|
||
| * Refs: | ||
| * Conditional exports naming usability discussion [#452](https://github.com/nodejs/modules/issues/452) | ||
| * Implement logical conditional exports ordering [#31008](https://github.com/nodejs/node/pull/31008) | ||
| * Stabilizing the resolver [#451](https://github.com/nodejs/modules/issues/451) | ||
|
|
||
| * GB: Ironing out usability. It’s best to merge with the next two agenda items (Implement logical conditional exports ordering + Stabilizing the resolver) | ||
| * GB: Resolver has hidden priority list - it will match the default condition last. If default is at the top, it doesn’t matter. Order did not matter. now `require` and `import` are on the same footing and mutually exclusive. Scaling exports (production conditions etc), as this list grows, the order becomes confusing. Potentially your environment matches multiple. So instead of relying on the resolver, we can give the user direct control by using object-order (defined in ES2015). Allows falling back if target is invalid, e.g. value is an empty array. Tobias had a nice example where you think of it as if-statements. As a programmer who writes if-statements, this naturally matches - seems nice. | ||
| * BF: JSON doesn’t assert an ordering. There are package utils that sort your package.json keys alphabetically. We need to be aware of this. Other languages - C# marshalling libraries do ordering based on target class, not the order inside of the JSON. Not sure if this is blocking. We should probably see if anybody big is doing this. | ||
| * MB: On ordering, keys with numbers could cause issues. So don’t allow keys that start with numbers. Seems reasonable-ish. | ||
| * JK: In Java, key-value structures, Jepson preserves key order. | ||
| * MB: Any objections to following ordering? | ||
| * BF: I want to do some research - no objection to the behaviour. | ||
| * GBO: Agree with Brad. Even if some things re-order them, is this preferable to current magic in the resolver? It would be no different to how it is now where you have to reimplement logic. It doesn’t make anything worse. | ||
| * BF: My concern with sorting… if we find something problematic… we could just say it’s alphabetically sorted (not package.json source text order). That would make it reliable. | ||
| * SAM: Json is a string. So a small patch to determine the order after parsing using a library, there are easy ways to use this. | ||
| * GB: These are the two things I want to do before shipping. #31009 is a bug. Self-resolve hits compatibility. If we add package name resolution to CJS, that is theoretically a breaking change. If we just ship self-resolve that intercepts the node_modules lookup and redirects to your own package, it’s breaking. So Jan made sure this applies after node_modules lookup so self-resolve only succeeds if it would have failed otherwise. My concern for implementing in ESM is it takes legacy into the future. I prefer better encapsulation, e.g. always load the local package. This PR says CJS will have the node_modules behaviour, ESM will have the encapsulated behaviour. | ||
| * JK: Import map bloat. We discussed opt-in in package.json. “I do want self-resolve”. Makes it safe for both | ||
| * MB: We did work to sync the resolver algorithms. This feels not aligned. Sigil seems good. We could add it later. I am personally uncomfortable with - deviating between CJS and ESM. | ||
| * GB: A major benefit of using the public name is that in a monorepo, others will use the same name, allowing import map to be de-duped. Sigil does not have - this advantage. | ||
| * MB: We should keep self-reference by name, but do it as last resolution. Sigil could be used as first resolution. | ||
| * CF: What happens if a transitive dep installs another version of the self package. Npm’s dedupe could cause another copy to land there. | ||
| * GBO: I like Jan’s suggestion of package.json opt-in, e.g. “exports”. | ||
| * CF: Could it work only if you have a direct dependency? | ||
| * MB: Self-resolve doesn’t even hit node_modules at all - it sticks within your package. | ||
| * GBO: That could have been a compromise? | ||
| * GB: I want to say self-resolve is enabled by “exports”. I need to think about it more, especially considering backporting. Might not be semver major. | ||
| * GB:Any naming concerns on conditional exports? | ||
| * *silence* | ||
| * GB: Ordering - we must ban keys that begin with numbers. Still need to talk more about self-referencing. | ||
| * GB: Stabilizing the resolver. My priorities: once we’ve got the above, we’re pretty good. Later, maybe refine error messages and validations. The imports feature is in Jan’s original spec - we could consider adding later. Benefit of self-resolve: allows internal conditions (private conditions). | ||
|
|
||
| ### unflagging in 12.x LTS [#450](https://github.com/nodejs/modules/issues/450) | ||
|
|
||
| * MB: I want to propose: assume conditional exports and named exports lands in January. We backport everything to Node 12. V8 7.8 is not on 12. So just wait. Goes out in Jan behind a flag. Remove the flag in April. We can update it monthly. Concerns? 13 is EOL in April. We can land breaking changes to an experimental API at anytime. HTTP2 is prior art. Just must avoid semver major breaking changes to stable APIs. I will take on the first pass on the backports. | ||
|
|
||
| ### Unflagging conditional exports [#31001](https://github.com/nodejs/node/pull/31001) | ||
|
|
||
| * MB: If we don’t have consensus on require(ESM) by January, I think we should unflag conditional exports so we have time to backport it. | ||
|
|
||
|
|
||
| ### Unflagging experimental resolve self [#31002](https://github.com/nodejs/node/pull/31002) | ||
|
|
||
| * MB: Similarly, for unflagging this the same timeline of January - potentially do this sooner. I will follow up with release time to push out the minor. Deadline is two weeks before the minor. Release plan for 12.13.2 should go out yesterday, but it did not. My guess is they slip by a week or two. So 7 Jan or 14 Jan is the cutoff - release team decide. | ||
| * GBO: require(ESM) is unlikely to land by Jan deadline - but maybe by April. | ||
| * MB: Jan 8 is the prudent date to meet. Can we do a call then? | ||
| * *silence* | ||
| * GBO: Anyone not available? | ||
| * *silence* | ||
| * MB: Will send an invite for an out-of-band meeting for Jan 8. | ||
|
|
||
| ### Loader Hooks [#351](https://github.com/nodejs/modules/issues/351) | ||
|
|
||
| * BF: Geoffrey introduced two hooks we haven’t talked about much. Framed about it being additive. I don’t think we are aiming to do bigger refactors currently. It’s not blocking a refactor in future. That’s it. | ||
|
|
||
| ### `require` of ESM [#30891](https://github.com/nodejs/node/pull/30891) | ||
|
|
||
| * WW: The first PR is the maximal one that could be produced. Highest compatibility. Allowed all ESM to execute as intended. So that we only walk back the parts people had issue with. On the PR, people don’t like event loop turns during what looks like a function call. Fair enough. But we don’t have to do that. We don’t have to allow sync execution of an async function. Browsers are considering banned Top Level Await - most modules don’t need it. So simpler solution is to synchronously unwrap resolved modules. It breaks the Promise spec, but TLA already does this. The way modules use it breaks it - you assume event loop turns between every module. It does not because the promises are synchronously unwrapped. A compromise so synchronous graphs stay synchronous. | ||
| * GB: The TLA spec executes modules in a synchronous way. It’s not promise based. It’s fully sync and only becomes async and promise-using if something needs it. If CommonJS were to load an async module, it would only find out at runtime. You’re suggesting it would throw here? | ||
| * WW: Two options. WHATWG spec machinery can premark a graph as containing async to make it an early error. Or just cheat a little and unwrap the promise. Then either return the promise or throw. | ||
| * GB: For consistency, you don’t want ESM loads to trigger re-processing. | ||
| * WW: You still continue the execution but the CJS require throws. Or you just return a promise from require(). | ||
| * BF: The PR intent was to allow things that look like ESM (but with different semantics) to work. We could do an early error without shenanigans. | ||
| * WW: I claim no breakage. The runtime can go beyond the spec. For ease of working, we just define it to always return a promise. | ||
| * BF: An early error seems non-objectionable. | ||
| * WW: WHATWG variant flags the graph as containing async. | ||
| * MB: I like the idea of failing early. This would preclude requiring WASM modules too. | ||
| * WW: The error message can be precise here. | ||
| * MB: Is there still multiple event loops? | ||
| * WW: No - I removed the inner event loop. | ||
| * GB: The other concern: loader hooks are async. Unwrapping promises includes unwrapping loader hook promises. | ||
| * WW: We wanted to move loaders off the main realm. By shuffling objects in a shared buffer with atomics.wait these are synchronous. | ||
| * GB: In those APIs, every resolve/source/translate must be fully synchronous. If we want HTTP loaders, the whole thread would be blocked whilst fetching. | ||
| * WW: We could promise.all it. Requires some re-design. Loaders must execute before the graph load. | ||
| * GB: With dynamic import this happens all the time and introduces pausing. | ||
| * WW: Or you just throw an error when you hit an async custom loader. Loaders interleaving with your program is scary to me. The third option: make all the loaders async with a sync adapter in front of it. | ||
| * MB: Could do an out of band meeting? Or discuss on the issue. | ||
|
|
||
| **END OF MEETING** | ||
|
|
||
| ## NOT DISCUSSED | ||
|
|
||
| ### Patch/Instrument a module [#339](https://github.com/nodejs/modules/issues/339) | ||
|
|
||
| * esm: The `getPackageType` utility function is not exposed [#30514](https://github.com/nodejs/node/issues/30514) | ||
|
|
||
| ### Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) | ||
|
|
||
| * 5 minute timebox |