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: #510 refs: https://docs.google.com/document/d/1XUBqEtPDwsgOgjrsoyacQhXHpTTdMyWAnEiwoZcDh4o/edit closes: #510
- Loading branch information
Showing
1 changed file
with
86 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,86 @@ | ||
| # Node.js Modules Team Meeting 2020-05-20 | ||
|
|
||
| ## Links | ||
|
|
||
| * **Recording**: | ||
| * **GitHub Issue**: https://github.com/nodejs/modules/issues/515 | ||
| * **Minutes Google Doc**: https://docs.google.com/document/d/1XUBqEtPDwsgOgjrsoyacQhXHpTTdMyWAnEiwoZcDh4o/edit | ||
|
|
||
| ## Present | ||
|
|
||
| - Myles Borins (@MylesBorins) | ||
| - Corey Farrell (@coreyfarrell) | ||
| - Jordan Harband (@ljharb) | ||
| - Guy Bedford (@guybedford) | ||
| - Rob Palmer (@robpalme) | ||
| - Wes Todd (@wesleytodd) | ||
| - Wesley Wigham (@weswigham) | ||
| - Geoffrey Booth (@GeoffreyBooth) | ||
|
|
||
| ## Agenda | ||
|
|
||
| ## Announcements | ||
|
|
||
| ### nodejs/modules | ||
|
|
||
| #### CJS exports detection [#33416](https://github.com/nodejs/node/pull/33416) | ||
|
|
||
| * GB: This took many years of discussion. Seemed to make progress on agreement in last meeting. Parsing approach is a candidate. Bradley made a project with Acorn parse + analysis. It became clear that parsing performance is important. Even 10ms is a long time. I put together a lexer (based on ESM detection) that detects a few patterns. | ||
| * GB: https://github.com/guybedford/cjs-module-lexer#grammar | ||
| * GB: If people webpack their code before publishing we can detect that. It does not do full scope analysis. TypeScript do something similar at top-level. Geoffrey ran it against the top-1000 modules. 62% were fully detected. Fallback is to only introduce the default export. It seems largely useful. I personally think it's useful to ship. | ||
| * JH: The PR proposes using `__esModule` that Babel produces. But Geoffrey's numbers do not use that. Searching for `exports.` won't work for all cases - `module.exports =`. I'd rather ship something that nothing. Want to maximize the patterns that work. The `module.exports =` is a perfectly valid thing to do. | ||
| * GB: We support React's pattern. And `module.exports.property`. Gus has put a block on this because if you have an existing module from `exports.` to a non-`exports.` you don't know you are breaking your consumers. Would like to decide this soon. | ||
| * MB: We landed in Node 14.3 the updated change to the errors so that CJS import of named exports gives you lots of good messages. It's a good step forwards from before. Having worked with wrapper modules, it's non-trivial, and makes it harder to test, then you need to sniff support. It's non-trivial extra work. You need to introduce package exports which opens up other things. Definitely doable but a lot of effort. So put strain on maintainers of lots of modules. With today's clear warnings, it's a lot better than it was. We can improve it even more with docs. I am not convinced the parsing is worth all the effort. I am -0 or neutral. Is the complexity and edge cases worth it? Do we think it's possible we can alleviate the existing blocks? Don't want people to waste time or get demotivated. Better to settle it before the labour goes in. | ||
| * JH: Speaking for Gus, adding exports is a potential breaking change. Adding Babel is too unless you are very careful. It would be unfortunate if adding this feature increased the set of accidental breaking changes. How can we show we're not making it worse? | ||
| * GB: The initial feedback was that people expected this to work - it works in bundlers. We can add this later and be non-breaking. But the support for the esModule flag is. Flag + default means we support Babel workflows. You don't want to `.default` from CJS. Cannot be changed once these workflows become solidified. | ||
| * MB: I think we need Gus to speak to move this forwards. Any other objections? | ||
| * JH: Depends on whether accidental changes are increased. | ||
| * WW: I appreciate we can move forwards with this and add more in future. | ||
| * GBB: I am concerned about the low numbers on detection. Over a quarter are not detected correctly. | ||
| * GB: Wonder how many of the 33% are just used by `require()`. | ||
| * WW: Lodash relies on ESM. | ||
| * GBB: There's a lot of big names in the not supported list. But they will likely get updated. So maybe don't worry about them? | ||
| * CF: I'm neutral. One comment about assigning an object to `modules.export` - that's how I roll. So maybe we can improve on this? | ||
| * GB: Yes, we can be additive. The grammar must be backwards compatible. | ||
| * GBB: I use it too. | ||
| * JK: Speaking of 33% I am less concerned about the numbers, so long as we capture 100% of the ESM-transpiled-to-CJS. I care less about hand-authored CJS. I would be in favour of scoping it to Babel/webpack output only. Gus treats this as something that doesn't fundamentally changes things, I think it does. Ordering of upgrades in a monorepo matters. | ||
| * JH: I want to know what the proposal's impact will be. "Here is the effective heuristic". | ||
| * GBB: Then we put it in the docs. I'd rather not scope this to Babel. Those are the one's most likely to be updated to real ESM. This is more useful for old packages that may never get updated. | ||
| * JK: I think the opposite is true because it affects the upgrade path. | ||
| * JH: Seems better to have a clear boundary. | ||
| * JK: End user is broken vs inconvenienced. | ||
|
|
||
|
|
||
|
|
||
| #### Unflag import.meta.resolve [#33464](https://github.com/nodejs/node/pull/33464) | ||
|
|
||
| * GB: We landed import.meta.resolve a while ago. This is the analog to require.resolve in CJS. It is async. Now we have top-level-await you can do this at top-level. We also have behaviour for a trailing slash to resolve paths. It was added to load an asset that isn't a module, e.g. a template. It's a nice useful convenience. | ||
| * JH: I am fine with the use-case. Like an API for the package root directory. I think CJS and ESM are first-class for the foreseeable future. Resolution is not unique to a module system. Whatever we add needs to be for both. Don't conflate resolution with... I expect resolve in ESM to only give me things I can import. Same rules should apply to resolve and import. | ||
| * GBB: I understand the philosophy but there are so many differences. Any unknown file can be require()'d. | ||
| * JH: But the content is a property of the file. | ||
| * GBB: This is a different system that uses URLs. These don't resolve to disk. | ||
| * GB: Resolving unknown extensions in CJS does not work. | ||
| * GBB: My point is that we don't need a 1:1 match. | ||
| * JH: People file bugs on resolve all the time. It has the same name. | ||
| * MB: There is a WHATWG proposal 3871 `import.meta.resolveUrl` | ||
| * MB: https://github.com/whatwg/html/issues/3871 | ||
| * MB: It feels a bit premature for us to unflag this whilst a standard is being discussed about `import.meta` that is shared across environments. Top-level-await is behind a flag. First there was "let's not call it resolve". Maybe not naming it resolve would be better. If we want to share it, perhaps consider putting it on util - allows us to deprecate it in future. It's host-specific, but for shared things we should try to coordinate. TC39 doesn't own this. If there is an import.meta.resolve I would want them to be the same. URL created a huge amount of legacy for Node. Putting it on meta does not accomplish all our goals. If we want to say it is node-specific, maybe call it `import.meta.node.resolve`. But maybe no advantage over util. | ||
| * GB: In terms of getting buy-in, the driving use-case exist in server engines, Node has the first mover advantage. I'll post an issue on Deno. I would be careful being conservative. We should serve our users. `import.meta` is contextual - follows self-resolution. It's statically analysable. It's hard to build Node apps to detect assets. `require()` can be reassigned. It helps uses do something they can't do today. | ||
| * JH: But only for ESM users. | ||
| * GBB: In webpack, builtins are not maintained. But static syntax is. Leads to simplicity which aids adoption. | ||
| * GBB: Could we ship this unflagged as `importResolveUrlFromModule` utility function. Then if standardized later we add it to `import.meta`. | ||
| *no conclusion* | ||
|
|
||
|
|
||
| #### package.json resolution and exports [#33460](https://github.com/nodejs/node/issues/33460) | ||
| * MB: People keep getting bitten by this. | ||
| * WT: The original author came to Node Tooling slack to talk about this. It is very common to import pkg.json. Now if you have a tool drilling through the tree, exports breaks this. Other solutions exist in the thread. All of them place burden on toolmakers. Because exports is new, it's odd to break everyone and now tool makers have to update their code. | ||
| * JK: We knew about this problem. The pushback is not from the wide community. It's an import for a small part of the community. We can ask those few tool writers to adopt the new resolution feature in the interest of the vast majority of users. I personally don't care as much unless we make it a clear exception - not overridable. Which means nobody can decide how that magical specifier can be resolved. Makes exports harder to explain. Might be acceptable trade off. | ||
| * MB: exports is a new thing with edge cases. Today's expectations are based on old ways that may go away. Caveats makes educating people harder. I am not a fan of turning it on for that reason. | ||
| * WT: Other people feel like that. The concern is that in order for module authors to adopt this, they must all expose package.json to work with existing tools. | ||
| * GBB: Don't think that's true. | ||
| **cut short - will return at the next meeting** | ||
|
|
||
| --- | ||
|
|
||
| * Chartering the Modules team [#412](https://github.com/nodejs/modules/issues/412) |