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

Commit

Permalink
doc: meeting notes for 2019-04-10 (#315)
Browse files Browse the repository at this point in the history
  • Loading branch information
MylesBorins authored May 8, 2019
1 parent 390b689 commit 7754e0c
Showing 1 changed file with 90 additions and 0 deletions.
90 changes: 90 additions & 0 deletions doc/meetings/2019-04-10.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Node.js Foundation Modules Team Meeting 2019-04-10

* **Recording**: https://www.youtube.com/watch?v=eCAcQ-r0CfY
* **GitHub Issue**: https://github.com/nodejs/modules/issues/310
* **Minutes Google Doc**: https://docs.google.com/document/d/1t9sYWYkDEAZKMhj-OGNlCeVyPjxOVYpscyOEury3Vhw/edit

## Present

- Geoffrey Booth (@GeoffreyBooth)
- Rob Palmer (@robpalme)
- Jordan Harband (@ljharb)
- Saleh Abdel Motaal
- Wes Wigham
- Guy Bedford
- Gus Caplan (@devsnek)
- Jeremiah Senkpiel
- Jan Krems


## Agenda

* Locking down the "process" and "Buffer" globals [#235](https://github.com/nodejs/modules/issues/235)
- GB: Only proposing to deprecate in ESM. Want to find out if we can keep the door open to this possibility. Once exposed, can never take back. Exposes root level security access. Do we want every package to have access to this? We have already made them getters on Node 12. Matteo has blocked this feature - so I need to work with him next.
- JH: Agree - once unflagged they must remain, and we can remove them up to that point. The roadmap to a new security model is questionable - a long discussion. Eliminating `Buffer` is fine. `Process` is problematic. Maybe just strip some properties?
- SAM: Platform detection / removing `process`. Worry about copy-paste. A frozen proxy could be sanitised in module scope.
- GB: Security/usability are important and need fleshing out another time. Saleh, this work is a path to the restricted process. Common detection cases so need to workout those workflows. Jordan provoked me to talk to Deno. Response was "global access is not a good pattern". React uses require() to allow dev vs production switching. Build tools can deal with this.
- JH: Deno already has a global. Ryan think many things are bad - there is not broad agreement on this list
- Jan: You could import `process`.
- JH: `typeof process` matters
- SAM: You may not know you are in a module. Does not satisfy `import` dynamically failing.
- WW: Removing unsafe globals is good. However, compatibility. ESM introduction does not make this ok. We must present the same APIs. It's unlike `__dirname` which are module-related. process and Buffer are not module-related. Removing them is not ok.
GB: We put process/Buffer into a scope wrapper for performance optimizations. When you access it, we do a stack-based check to see if you are in a module. It is not a good pattern. We could not do this for `import "fs"` or property access. Global access is syntactic.
JS: Is the global object accessible via `global`?


* Flags functionality options [#300](https://github.com/nodejs/modules/issues/300)
- refs:
- Replace --entry-type with --input-type, plus --input-type=auto [#68](https://github.com/nodejs/ecmascript-modules/pull/68)
- GB: I have heard objections. For string input, the user may not know the type. coffee/babel do not use eval. As we know, the syntax is ambiguous. Unambiguous case with import/export is handleable. For the ambiguous case, maybe look for CJS globals? Consensus was that this was too messy. So we redefine the goal, and ambiguous files fall back to CJS. Would like it to be added to `VM.createScript` - the one that Babel uses so that tooling becomes consistent.
- JH: It's fine to ask the VM "is this string unambiguous?". Some of TC39 from Browsers objected to auto-detection is that mid-way through typing a file, the mode would switch and the users would be surprised. "auto detection is a footgun"
- GB: This behaviour is different because you must opt-in to the behaviour.
- GC: So this is for something where you don't know the type so the smartest you can be is detecting import/export means you are NOT sure and have NOT solved the problem.
- GB: At least it mostly works for the entry file.
- GC: I don't imagine I will be in this situation.
- JS: Want to avoid any form of auto-detection. Maybe have it as a function to ensure consistency? Though I think anyone using this is laying a landmine.
- GB: The use-case is REPL-like. Users don't want to specify.
- JS: The REPL is special.
- GB: I will refactor the PR into a function on `module` that tools can use. It's a starting point. Any objections?
- GC: I am not a fan. But keep it behind a flag and I'm ok with this.
- GB: e.g. --detect-input-type

- Replace --entry-type with --input-type [#66](https://github.com/nodejs/ecmascript-modules/pull/66)
- GB: Current implementation is a footgun IMO. People expected it to be package-scoped, not just apply to the initial file. Initially we proposed `--package-type` but there were objections. Then people suggested eliminating the flag. So maybe we only have it for eval/STDIN not for file, i.e. scale it back.
- JH: Strictly critical case is string input - otherwise parse goal is uncontrollable. Standalone file case can be handled by `.mjs` vs `.cjs`. Remaining case is extension-less file.
- GB: Symlinks can handle that. It's a small price to pay. Good to ditch the footgun.
- JH: We just need to explain ourselves. Seems reasonable to tell people to add a `package.json` with a field. We should be clear: extensionless file will be annoying.
- SAM: npm installation gets symlinked into .bin
- GB: This only happens if you already have a `package.json`. Users of --preserve-symlinks get burnt. You can't run npm with this flag. Let's go ahead and see if users speak up. It's a rare use-case. Consensus to merge the PR? (does not include `--input-type=auto`)
- CONCLUSION: We have consensus to merge!

* Dual Mode Packages
- refs:
- Proposal for single-mode packages with optional fallbacks for older versions of node [#299](https://github.com/nodejs/modules/issues/299)
- WW: Concept: Some people said importing must be async, preventing substitutability - you can't `require(ESM)`. We have used this as an assumption for a while. I have been unhappy. Gus did work syncifying arbitrary promises. Spec doesn't force loaders to be async. Today, Node implementation is async. Syncifying permits deadlock. But this doesn't matter for loaders in Node. So we have a choice: sync vs async. So we can go back on the long-held assumption. Enabling substitutability. Eliminates multiple versions of the same module. So we can side-step this problem.
- GB: These two proposals aren't really counter-proposals. Wes is encouraging require(ESM). That should be a standalone PR. The other part is about reusing the "main" fields, which I am not a fan of.
- WW: Once you can require(ESM), things become simple.
- GB: People like me want to publish things usable in old versions of Node. Overloading main doesn't work with source vs dist packages.
- JH: Source vs dist is already a pain point. We should solve it for both CJS and ESM - or not solve it at all. Want some way to do dual mode packages. I like extension resolution. source vs dist in CJS is solved via top-level modules that resolve on disk. I want to avoid two instances of the same module in memory for the same module on disk.
- WW: Agree. Exports is a good thing to apply to CJS and ESM. A prettier solution that is limited to ESM is limiting. Old versions of Node already have a single entrypoint. Offering this for new versions of Node is fine.
- SAM: Syncifying. If we can do this, we really need to explore it. Can we work towards a model and see if this works?
- WW: This exists.
- JS: Dual-mode packages is invalidated if you have substitutability (by eliminating sync vs async). Top-level await may arrive.
- WW: This doesn't matter.
- GB: Let's have a dedicated dual-mode meeting after Wes gets the PR up.
- RP: But TLA does matter.
- WW: `require("ESM")` will return the namespace object immediately, even if the module's evaluation has not yet completed and therefore the bindings may not be populated at the point it returns.


**END OF MEETING**

**NOT DISCUSSED**

- esm: scoped --type, cpp refactoring [#57](https://github.com/nodejs/ecmascript-modules/pull/57)
- Warn about `--type` with shebang [#37](https://github.com/nodejs/ecmascript-modules/pull/37)
- Proposal for dual ESM/CommonJS packages [#273](https://github.com/nodejs/modules/issues/273)

* Proposal: Support loading package by own "name" [#306](https://github.com/nodejs/modules/issues/306)
* Exports main [#41](https://github.com/nodejs/ecmascript-modules/pull/41)
* Moving forward with Dynamic Modules? [#252](https://github.com/nodejs/modules/issues/252)

0 comments on commit 7754e0c

Please sign in to comment.