Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

doc: august meeting notes #380

Merged
merged 3 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions doc/meetings/2019-08-14.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
# Node.js Foundation Modules Team Meeting 2019-08-14

## Links

* **Recording**: https://www.youtube.com/watch?v=OEFVWjpkbao
* **GitHub Issue**: https://github.com/nodejs/modules/issues/370
* **Minutes Google Doc**: https://docs.google.com/document/d/1Ng0-0Mp34z2gAqkDsyWsZkFK6XUIrgsyA3lv8i4917U/edit

## Present

* Myles Borins (@MylesBorins)
* Gus Caplan (@devsnek)
* Jordan Harband (@ljharb)
* Jan Krems (@jankrems)
* Saleh Abdel Motaal (@SMotaal)
* Bradley Farias (@bmeck)
* Alex Aubuchon(@A-lxe)
* Geoffrey Booth (@GeoffreyBooth)
* Guy Bedford (@guybedford)
* Daniel (@???)
* Darcy Clarke (@darcyclarke)

## Agenda

## Announcements

*Extracted from **modules-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting.

### nodejs/node

* esm: support loading data: URLs [#28614](https://github.com/nodejs/node/pull/28614)

Bradley: We talked about data URLs last week. No objections on the PR right now, no quorum, no moratorium. Can we go ahead with this?

Myles: I think we're good to move forward

Jordan: This feels like something that will be abused more than used, and the benefits are minimal compared to the risks. Justification seems to be "the web did it so we should too" which doesn't sound sufficient to me. I'm not clear on what the use cases are when you have a filesystem, unlike the web where you don't have a place to make a temp file on the fly.

Bradley: We have a few use cases in the PR. Particularly when you don't have write access to the file system.

Myles: On GCP we don't offer file system access -- none at all after build.

Saleh: It's useful to see the use cases, and so as long as it's flagged, we can see use cases. Also, cross node/web functionality allows for more consistency between solutions (ie loaders). Node currently forces you to write virtual modules to disk.

Jan: I talked with somebody yesterday about on-the-fly content generation, and they cannot rely on disk (security concerned, can't trust file system writes). At groupon the temp directory was not guaranteeable across environments.

Jordan: Ok so even if all these use cases *can* be done with the filesystem, the filesystem isn't always available, so data urls become necessary.

Geoffrey: For security concerns, you might want to avoid writing to the file system as that exposes it to disk issues, other processes looking at it.

Bradley: I think we should avoid security concerns as they're difficult to puzzle around given Specter.

Jordan: @jan commonjs requires the filesystem?

Jan: The require system is open to all things, so you can generate a module from string.

Jordan: Are there plans to add data URIs to cjs?

Jan+bradley: No because you can do it anyways, and also the cjs specifier system doesn't work well with URIs.

Myles: Taking a step back, we have an upstream PR, but no upstream objections. Are there any?

Bradley: It sounds like we should keep data uris behind a flag

Jordan: I think it should be behind the experimental modules flag -- it would be a waste of effort to make a new flag right now.

Myles: I don't think we need a flag, but it's not much work to add one. It would be weird to not have a flag and add one later, so if that's the plan start with it. I prefer no flag though.

Jordan: I'm not fully in favor of data uris, but I do not want to obstruct the PR.

Myles: How about we merge it, and then Jordan opens a revert PR arguing why.

Jordan: Give me a week or two to consider if the revert is needed, but SGTM
GeoffreyBooth marked this conversation as resolved.
Show resolved Hide resolved

### nodejs/modules

* Validate exports keys and values [#359](https://github.com/nodejs/modules/issues/359)

Jan: Landed already

* Package Exports [#341](https://github.com/nodejs/modules/issues/341)

Jan: made a lot of progress implementing exports, behind an additional flag automatically enabled if using --experimental-modules. Progress board exists. Implementation supports most of the proposal. `.` "main" discussion ongoing. Import map names still being discussed as well. Would like to gather consensus on those before unflagging.

Jordan: Would love to see this land in CJS, tons of uses.

Myles: would this be moved out of experimental if unflagged or stay experimental. Prior art allows us to unflag while keeping experimental.

Jordan: If we are unflagging it, it should be considered stable. That is when people can use it reliably without opting in. Stable enough to risk no further changes possible.

Saleh: exports written for CJS. even if ESM is flagged would this be available in CJS without a flag?

Jordan: yes

Jan: After unflagging exports should remain experimental potentially because it has ecosystem effects. Important to get feedback from other environments like bundlers as ecosystem support is picked up/considered. As ecosystem adopts, changes may need to occur with care.

Myles: we can keep the console warning even without the flag when it is used. I am comfortable calling things stable even with these warnings.

Jordan: I was mostly talking about unflagging ESM, unflagging exports is not as much a concern. When unflagged ESM will be risking things being kept as-is. We can prepare the ecosystem for unflagging by adding lint rules and support to 3rd party resolvers.

Myles: the stability being experimental allows smaller edge cases like error codes to be changed without a semver major.

Jordan: since exports affects 3rd party code, the warning would be coming from 3rd party module installation.

Jan: minor issue with exports is that it cannot be unit tested by the package itself. This should be fixed.

Jordan: this is a problem in general of "testing your package as if it were published".

Myles: loading via relative specifiers inside the module is inconsistent vs "exports"

Jan: See "imports" proposal from guy for internal aliasing.
… talking about using ~ to get current package ...

Jordan: might want to enforce ~ going through exports. E.g. package.json#"files" is dangerous because forgetting to add files to the whitelist.

Myles: seems like we are ok with "exports", flag removed separately from ESM. We should identify a new feature/propose something for ~ which we talked about. Should we sync/move package name proposal from seeking consensus to implementing?

* Loader Hooks [#351](https://github.com/nodejs/modules/issues/351)

Brad: Lots of comments in design doc + github. Adding success criteria to examples. Need to sync up on changing the hooks themselves. Lots of suggestions and different directions. General feeling that we should split up the hooks somewhat. A few ideas on what the divide to be. Fairly consistent feeling that resolve return location and a seperate fetch hook that turns a module record or resource for module record is an approach being talked about. Figuring out how to split that and start moving.

Still questions about how loaders should defined their format. Design document has a mime for determine format. Current loader is using a string enum. Contention on which approach should be used. Likely something we need to reach consensus within the committee. Don’t have quorum, so perhaps we need to talk about it again. Good thing to start talking about.

Guy: Thread on loader discussion looks [?]. Lots of agreement on things, not much contention. Biggest thing for a usability perspective is separating the hooks. Happy to see we can look at the design. Mostly usability concern regarding mimes, but it might be superficial. A little bit concerned we don’t have a mime type for a node builtin, or what a .node file would be. So we need to come up with something for those cases.

Brad; we have “application/node” for cjs. We tried to standardize something for C++ modules but it was rejected as they don’t have a well defined format. Could look at vendor track rather than standards… just need to agree with what to call them.

Guy: I’m completely happy to go with mimes, no problem moving forward with that.

Brad: Working on the way to have composed instrumentation of builtins. Added example but it may not cover all examples of what people can do. Basically the contrived example is adding to instrumentations to fs.readFileSync. Adding a console.time and console.endtime to it. If you have two loaders doing this it is hard to build right now, need more design work on this. Guy showed an idea of using import.meta to pre-inject other module namespaces so they can be used without a lot of redirection or source code rewriting. This is one of the harder solves

Another built in experiment we have is the null loader. A loader that doesn’t load something. Example preventing loading fs. Synthetic module loader is another example, something created from memory. A transpiler loader, success criteria includes having a custom format sent through the pipeline. You need to implement regular expression modules.

Are there specific use cases we need to have?

Geoff: glancing at design doc for first time and I see the new examples at the end. When ready would love to see an example of DX. I see code but where do I put it? How do I let node know to use the loader? How do I create / use a loader?

Alex: @Geoff the --loader api is the same as the way cjs loaders are currently loaded with --require, so we can look towards them for usability. One difference is that with cjs, loaders can often also be loaded in the middle of runtime with require(...), which we are not allowing for esm loaders due to security, predictability concerns.

A use case I haven't seen considered is that of loading a v8 compile cache (https://github.com/zertosh/v8-compile-cache). The current proposal does not support dynamic instantiation of modules (only returning their content so that node can do the instantiation). Given that, the only way I can see supporting compile cache is by adding a compile cache content format that node supports. This is outside the realm of loader hooks, but should be kept in mind.
Loading