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: #461 Refs: https://docs.google.com/document/d/1RwdeqHV9oJBBafcAajWZQ0eCnzcMNSo-dQGN1WeEszY/ Closes: #461
- Loading branch information
Showing
1 changed file
with
126 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,126 @@ | ||
| # Node.js Foundation Modules Team Meeting 2020-01-08 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/461 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/1RwdeqHV9oJBBafcAajWZQ0eCnzcMNSo-dQGN1WeEszY/ | ||
|
|
||
| ## Present | ||
|
|
||
| * Myles Borins (@MylesBorins) | ||
| * Bradley Farias(@bmeck) | ||
| * Guy Bedford (@guybedford) _GB_ | ||
| * Rob Palmer (@robpalme) | ||
| * Jan Krems (@jkrems) | ||
| * Saleh Abdel Motaal (@SMotaal) | ||
| * Corey Farrell (@coreyfarrell) | ||
| * Geoffrey Booth (@GeoffreyBooth) _GBB_ | ||
|
|
||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| * Extracted from **modules-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. | ||
|
|
||
| ### nodejs/node | ||
|
|
||
| #### module: implement logical conditional exports ordering [#31008](https://github.com/nodejs/node/pull/31008) | ||
|
|
||
| * GB: We have a PR up for a big change to the way exports are matched. Current implementation is based on a hidden list of export conditions in the resolver. When resolve looks at conditions, it would match them in its internal order. Concern was: if user is using many conditions, the priority order is hidden. So resolvers could behave in different ways. Hard to determine what will match. PR switches to "object order" as defined by package author. Also, for nested conditions it will fallback - directly think of them as acting like `if` statements. Hopefully this is more predictable. Control is back in the hands of the package author. This PR has been merged to Node master. | ||
| * BF: Please confirm the previous questions about JSON order and relation to ecosystem tooling. It's a well-defined ordering. We have found no ecosystem issues. `OrdinaryOwnPropertyKeys` is the official algorithm. | ||
| * GB: Gus requested array indexes do not follow the same ordering. Numbers come first, then insertion ordering. So userland tooling could get this wrong. The PR addresses this by adding validation, to ensure there are NO array indexes in the object, otherwise throw an error. It's fully well-defined and aligned with the spec. Node achieved consensus on this. I feel `exports` is now feature complete. It's really nice. The only outstanding PR is unflagging. | ||
|
|
||
|
|
||
| #### module: unflag resolve self [#31002](https://github.com/nodejs/node/pull/31002) | ||
|
|
||
| * MB: Landed. Released in 13.6. | ||
| * JK: Congrats to Guy Bedford for making this happen. | ||
| * GB: It's backwards compatible with CJS. We compromised by saying the feature is only active when `exports` exists in `package.json`. It's another justification for preferring `exports`. | ||
|
|
||
|
|
||
| #### module: unflag conditional exports [#31001](https://github.com/nodejs/node/pull/31001) | ||
|
|
||
| * MB: Logical Conditional Exports landed. So this follows on. We considered require(ESM) as an alternative. We think one does not preclude the other. I am in support of landing this unflagging. | ||
| * GB: I am happy with where we have got to. | ||
| * MB: About to have 3 upstream LGTMs. Just need to run CI. Any objections to landing this today subject to green CI? | ||
| * JK: I think yes. My memory is that we were supposed to discuss... I feel bad making this call when Wes is not here. Doesn't feel great to call it now. Would prefer his awareness. | ||
| * MB: We're only talking about unflagging, so it seems reasonable to wait. | ||
| * BF: How about we reach consensus here and then wait on Wes for a reply? I would like to get consensus here to avoid back and forth meetings. | ||
| * MB: Any concerns with the Wes reach out approach? | ||
| * GB: Hope we don't wait indefinitely. Waiting one week seems sensible. | ||
| * MB: Ok, let's wait until next meeting for his response. | ||
| * GB: Sure. | ||
| * MB: I don't think this affects release timeline for backports to 12. It won't change behaviour - just unflagging. | ||
| * GB: That still affects people's ability to publish dual-mode packages. | ||
| * MB: Ok. | ||
|
|
||
| #### esm: implement import.meta.resolve [#31032](https://github.com/nodejs/node/pull/31032) | ||
|
|
||
| * MB: We are trying to align with WHATWG implmentation. | ||
| * GB: Browsers have discussed for a while. In Node, this is the equivalent of require.resolve. This is an important convenience feature. The PR calls the resolver, including hooks. You can provide many types of specifier. Relative, package name, etc. Useful for asset workflows. It is **async**. We chatted about this. Async feels safer. Allows us to syncify in future. A hazy argument for this being a slightly safer path. In Browser, it must be async for a few reasons, e.g. dynamic import map injection. I sent the PR to Hyrishi and Domenic. They agreed async could be alignable. Hard to get their final position because browsers may change. Do we want to take the leap first? Or be conservative and follow the browser. I would like to land. But will understand if others do not want to land. | ||
| * BF: This is our first meeting on this topic. I am not comfy landing this unflagged for a while probably. There are things happening in the Modules Group that may need to hook into this. We don't have a way to pass in conditions, but we are using them for logical ordering. There's also TC39 module attributes happening. Lots is happening. This here is too simple. In future we wil need to replace it to have the utility of a resolver. We need a much more feature rich API later no matter what. | ||
| * MB: I think this is premature to land unflagged. Before implementation, we can work with WHATWG and HTML on spec text. I am concerned about unflagging before we have spec text and clear consensus across groups. | ||
| * GB: On interactions with conditions, conditions are environment-specific constants of the environment. A generic resolver would need many things - a super resolver. That would be very different to this. One is a library. The other is the local environment resolver. It's simpler. This import feature matches the current environment. On module attributes relating to resolution... | ||
| * BF: They affect allowable forms. | ||
| * GB: I think it's dangerous to have more metadata to affect resolution. This is one of the few situations where it can just ship things and then follow up with breaking changes later. This is one of the only cases where Node.js is in this kind of position. It may even steer browsers and accelerate things. It makes it easy for Browser to follow us. Otherwise we risk losing a year. | ||
| * MB: Breaks in a flagged or unflagged? | ||
| * GB: Unflagged. | ||
| * MB: I don't think that's acceptable. | ||
| * BF: We're discussing interop and breakage. Maybe just don't put it on import.meta. We made createRequire. We could make createResolver. This PR allows you to specify another URL to resolve against. You can impersonate URLs. So it may be ok to say "you have to import this". | ||
| * MB: If we make this an experimental API on Module, that appeases my concern. If we make it a slightly lower API, it could become part of the official import.meta later. | ||
| * GB: I have needed to use asset references. import.meta is statically analyzable. Most build tools don't do the work to analyze imported stuff. The kind of breaks we might expect are "we'll support the API, but not the second argument, and it's sync" - this would only break 10-20% of users. | ||
| * MB: We could do both. Keep it flagged on import and unflagged on the lower-level. | ||
| * GB: Agreed. | ||
| * MB: I want to encourage experimentation and reaching out to WHATWG. Want to avoid mass breaks. | ||
| * GB: I can flag the PR. | ||
|
|
||
| #### WIP: Move ESM loaders to worker thread[#31229](https://github.com/nodejs/node/pull/31229) | ||
|
|
||
| * BF: We have previously discussed moving loaders to other realms and threads. This PR has a long way to go. We can base discuss around this. Hooks are moved into a separate thread - but no API change except it removes dynamicInstantiate because that is tied to the Realm. This PR is a way for us to discuss having them on a separate thread. Instead of each thread getting their own Loader instance, instead the thread is shared amongst all works + the main thread. This is a new type of thread - an internal work. So it is shared and can receive requests from all threads. We are probably going to need add hooks. We can't intercept the --require flag. We don't have a clean way to instrument globals. We'll need to do that. I want this PR to be blocked on all those other landable things. I want to say "in order to land this PR, we need xyz feature" then we raise a PR for the feature and block this PR on that PR. | ||
| * CF: If this PR is open for a while, does it make sense to separate out dynamicInstantiate? So that feature can get removed before it gets popular. | ||
| * BF: It's possible, but I do not see large benefit. For GetSource we have an awkward API signature. The resolve can still return a separate format. We can mark it as "soon to be removed" but I don't see removing it helping users right now. | ||
| * CF: Then let's document it that way to let people know. | ||
| * BF: It's already in use. People use it to avoid weird source code transform. When they get the GetSource hook we'll see if they transition over. | ||
| * JK: I agree with Corey. Would prefer to remove it sooner or at least add a deprecation warning. Also, do we have quorum on moving to an isolated context? We could make a call on this now. | ||
| * GBB: Putting each loader in a separate thread... I assume we can still sequence loaders. Babel->CoffeeScript. What does this PR preclude? | ||
| * BF: The big things are the isolation. Today, if you create a global property, you can see it in user code too - and can interact between app and loader. With this PR, that interaction is not possible. Leaving you to perform message passing. Sharing globals is probably not a good thing to do. Today we spin up 5 coffeescript loaders if you have 5 threads, and they may need to coordinate. | ||
| * GBB: Test runners? | ||
| * BF: We do not have a good solution to test runners. The bigger problem for utilities is the module cache - we do not expose it in any form. | ||
| * JK: Do we have consensus on moving loaders to a separate context from user code? This is the highest order decision. | ||
| * BF: You can only share the loader if the loader is isolated from the app. | ||
| * GB: Worker threads? | ||
| * BF: It creates a new message channel to the same internal worker that hosts the loader. The browser has a similar concept. | ||
|
|
||
| ### nodejs/modules | ||
|
|
||
| #### Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) | ||
|
|
||
| * MB: We have never discussed this in a meeting. My intuition is that we have everyone we need to charter, aside from small bits. And if we do charter, what do we want authority over. | ||
| * GBB: For the big releases, we had to break our own rules or thought about dissolving the group. If we charter we can't do that. | ||
| * MB: We could decharter in that case. Right now, we are acting as though we are chartered. Suggesting we should charter. Maybe we don't need to dig into this. Would be good for people to chime in. | ||
| * GB: What are the benefits of chartering? If there are no risks, go for it. | ||
| * MB: Being officially recognised from a process perspective would allow members to block things upstream. Could stop the tsc unilaterally making changes without us chiming in. Chartering process is about setting expectation. My previous experience has been with the Release WG which had external work not inside core. Matteo Collina might have thoughts for us. | ||
|
|
||
| #### Loader Hooks [#351](https://github.com/nodejs/modules/issues/351) | ||
|
|
||
| * GBB: I worked on this with Brad. There's more to be done. Check out latest master. There are four hooks. Resolve. GetFormat. GetSource. TransformSource. I have not reviewed the long list of loaders. There were concerns from Guy and Bradley. The biggest outstanding thing is test runners and stubs. If people have ideas, please share. | ||
| * BF: Previously people did not want multiple loaders. | ||
| * GBB: Does the thread change impact things? | ||
| * BF: Just make sure the interchange between the hooks is JSON-serializable. You don't have to worry too much. | ||
| * CF: On multiple loaders, I do think there needs to be a way to accomplish this. I am concerned that solving this in userspace might be problematic. So I would like to see more discussion on this. | ||
| * JK: I am skeptical about multiple loaders. Because the more complex scenarios, where loaders compete to affect the same things, generates weird scenarios where the order of running the hooks becomes too complex. I am nervous about this. | ||
| * GBB: Think of Gulp. gulp.parallel. gulp.series. Having Node do this... Either it's simplistic, or userland will do something to orchestrate multiple loaders. So how sophisticated should Node be? Don't want to compete with Gulp. | ||
|
|
||
| --- | ||
|
|
||
| **END OF MEETING** | ||
|
|
||
| --- | ||
|
|
||
| ## NOT DISCUSSED | ||
|
|
||
| * Stabilizing the resolver [#451](https://github.com/nodejs/modules/issues/451) | ||
| * unflagging in 12.x LTS [#450](https://github.com/nodejs/modules/issues/450) | ||
| * Patch/Instrument a module [#339](https://github.com/nodejs/modules/issues/339) |