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

Latest commit

 

History

History
183 lines (100 loc) · 13.5 KB

2019-08-28.md

File metadata and controls

183 lines (100 loc) · 13.5 KB

Node.js Foundation Modules Team Meeting 2019-08-28

Links

Present

  • Modules team: @nodejs/modules
  • Jan Krems (@jkrems)
  • Wesley Wigham (@weswigham)
  • Myles Borins (@Mylesborins)
  • Saleh Abdel Motaal (@SMotaal)
  • Guy Bedford (@guybedford)
  • Darcy Clarke (@darcyclarke)
  • Rob Palmer (@robpalme)
  • Jordan Harband (@ljharb)
  • Gus Caplan (@devsnek)
  • Hassan Sani (@inidaname)
  • Geoffrey Booth (@GeoffreyBooth)
  • Jeremiah Senkpiel (@Fishrock123)
  • John-David Dalton (@jdalton)

Agenda

Announcements

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

nodejs/modules

  • Divergent specifier hazard #371

GCB: When I was working on the attempt to try to get dual packages (the same package that contains both CJS and ESM sources being able to do require(pkg) as well as import(pkg) and it works in both cases). The user has to write require(pkg/cjs) import(pkg/esm). So we didn’t want to have two specifiers. If we have one specifier mean two different things in the two module systems this can cause chaos. And this has already come up in the GraphQL project where they adopted mjs under the previous --experimental-modules. I didn’t fully understand the issue so I dug into it a bit and tried to come up with a minimal repo of what the issue was. My sense was that this isn’t just a dual package thing but would also apply to extension searching, file.js and file.mjs having import ‘./file’ and require(‘./file’) resolving to two different things is basically the same issue. We basically have three options - (1) what we have now, (2) allow a hazard and document it properly or (3) come up with some clever solution to avoid the problem. The best one we have at the moment that came up in the thread is to have require support requiring esm, so that both would get esm sources resolving the issue. There’s a lot of issues separately with getting require(esm) to work.

JHB: The hazard you are describing is one that has always existed in npm, not just because of extension searching but also because of package duplication which is a common thing in package ecosystems. The solution to this is peer dependencies which ensure there is only one copy in the tree. This is not something that can be fixed by leaving things as they are. If you use that solution then extension searching and dual packages offer no additional hazards. For me I have not seen any examples of a case that differs from that, so this hazard should not be relevant to discussing dual packages or extension searching. My understanding of the graphQL issue was having one in CJS and one in ESM.

JK: I think it is skipping over some finer details to say that this is nothing new. Peer dependencies cannot save you from this assuming people are publishing packages that have both which is the only scenario that is actually a thing. Even if you only have a single copy of the package, because dynamic resolution applies you may find that you have two copies of the same module implementation within that same package. So there are ways to handle it by having specific files not specifically duplicated within the package. If you only transpile parts of the package and then have to manually maintain those parts of the package is definitely not something that the ecosystem is used to.

JHB: That is fair thank you Jan.

WW: I agree that the hazard exists, however the current tooling and recommended practices for working with CJS and ESM involve shipping both versions side-by-side. And changing peoples views on that is a monumental task.

Going back to what Geoffrey was saying. We don’t actually need full require of ESM. All you need to say is that the ESM resolver is a strict subset of the CJS resolver. So that CJS would do everything that the ESM would do. If the result is something that should have been loaded as an ESM file, the CJS resolver can simply say no and throw. That’s all you need to do. You just need to be able to know that you should not load the thing. Actually being able to load it would be a nice enhancement on top of that but that’s not actually the thing that resolves the problem.

JK: I assume that this implies ESM would never apply .mjs, or if it would it would do it with a lower priority than existing things. Otherwise it could never actually do it without stopping to be a subset of the CJS resolution.

WW: The idea is that the ESM resolver today sees mjs as needing to have the exact extensions. Yes, if there is extension searching it must be the same. But the priority can be changed in both at the same time with something like the type: module field.

MB: I want to step back a bit. Why Geoff brought this up was to look at why we don’t want to have extension resolution on by default, which does protect this hazard. To Jordan’s point, this hazard does exist to some extent within the ecosystem and is why npm started flattening dependencies specifically to minimize when this hazard would hit. And in my own personal experience when this hazard does hit it hits in ways that aren’t very intuitive. In order to understand what exactly is happening requires a somewhat sophisticated understanding of the Node.js module system. I don’t find that a compelling reason for us not to try and minimize the hazard in our own runtime. We’ve pointed this out as one reason for not having default file extension resolution, compatibility with import maps being another. Stepping back, the intention of opening this issue was to maybe close the discussion as to turning off extension resolution by default. If we want to discuss major changes for how we do things maybe that should be another discussion.

GCB: To clarify it was actually to close the discussion around dual packages, but that was more of a side effect.

MB: So you would like to remove extension searching as one of the cases that we are trying to solve for with our implementation?

GCB: It would be moved down into the tabled area of phase 3 where we are not going to pursue this further. The only way I see we can avoid that happening is the require(esm) solution. If that PR comes up in a month, two months, even after we unflag it could be unflagged.

WW: First of all, extension resolution, while it increases the surface area of this hazard, is not the only way this hazard surfaces. Anywhere the resolvers do things differently - ie in the interpretation of package.json flags. They should not be different resolvers, and the fact that they are different is the core of the problem. We just need to be able to guarantee that the one resolver always behaves as a subset of the other that is all. We’ve made this harder implementing the ESM resolver in C++ thinking that was a good idea, but if we were willing to just go back on that for a moment, then that would actually entirely solve this problem. Because both resolvers know about extensions, type: module etc. So the ESM resolutions can throw in CJS. And ESM can load CJS fine.

JHB: Although I have many feelings about dual packages and extension searching, the thing I think is really critical is solving the problem of how I can publish a module that can work at least across Node versions - that is across the barrier between supports ESM and does not. If dual packages or extension searching isn’t the answer then fine let's find another answer.

Anything that requires to write prose to explain it is just a massive failure in my opinion in providing compatibility and interoperability.

JK: ESM resolver is currently a subset of CJS. Anything that is not is a bug. The intention is for it to be a subset. A shared implementation might help guarantee that. But my attempts at sharing were not maintainable. Changing the CJS resolver is hard. I favour using a test suite to achieve compatibility.

WW: We added exports to both. The CJS resolve doesn't understand type:module - that's the big bug in the current implementation.

JK: After fixing this bug, we no longer face divergent specifiers.

WW: If we start with the subsetting, we can add extension searching, then add require(ESM). That is incremental.

Guy: But extension searching does not change the argument.

WW: Divergent specifiers does not imply no extension searching.

JK: This is about a specific form of searching - prioritizing mjs.

WW: type:module lets you opt in to the change in priority.

MB: The order of extension searching for CJS and ESM should be the same, and should switch based on type:module, means you can't just drop the extension to customize resolution behavior for each system.

WW: ESM would need to respect require.extensions.

JK: We are trying to get rid of that.

WW: We could try to remove require.extensions from both systems.

<discussion of resolution subsetting being orthogonal to require(ESM)>

Guy: If you try to require(ESM) you would get an error.

MB: If your "main" is extensionless, it resolves based on "type":"module". This solves a case.

Guy: ESM files loaded by the CJS system must throw.

MB: CONCLUSION: We will make the CJS resolver respect "type":"module", and extension searching order must be the same between both implementations.

WW: The upgrade path is currently garbage.

MB: Maybe we can target synchronous require in Node 14 as an incremental enhancement. Node 12 would cause breaking changes.

WW: We just have to make things throw now.

GB: We're not going to to implement dual packages before unflagging.

JHB: I don't think we should do that.

MB: We're at an impasse. Do you have an alternative proposal, Jordan?

JHB: No. I would like an explanation of how to write a package in Node 12 LTS that works in 8, 12 non-LTS, and 12 LTS without forcing "await import()".

GB: The only solution we have is the as-is.

JHB: But this requires reading documentation. I rarely read readmes.

JK: We know specifiers won't be modified, and tooling does not rewrite these for intermediate packages. We need to write that guidance.

GB: Shall we put the migration guide on the roadmap? Do we defer dual-mode?

JHB: Let's do the migration guide first.

MB: It's not reasonable to block unflagging without proposing an alternative solution.

JHB: Lots of people have blocked things.

MB: This is different.

ACTION: TLA in CJS?

ACTION: Migration guide

ACTION: Respect type: module in the cjs resolver

  • Revisit dot-main in exports field #368

JK: Exports originally had a dot main as a way to drop using main as one field for both. In the very first version there was even a shorthand where if you set exports to a string that would be the main. We dropped that (a) because of the dual package thing, and (b) dot main happened to be somewhat contentious as a way of having a key that was just a dot. But now that we have additional syntax in exports such as the array fallback syntax, we basically come to a choice between two semi-bad options. (1) bring it back - exports can override main completely both for CJS and ESM. or (2) we could support that same array syntax in main but that would mean that if you want to use fallbacks for your main export you would need to drop support for all bundlers before it was introduced.

My thinking is that it is best to go with dot main.

MB: Is the fallbacks support already supported in the implementation?

JK: It is already supported - the fallback takes the first thing it understands, the first URL. So the current implementation does support it. And if we want to support it at anytime in future we do need to support it right now as the point of fallbacks is being able to have support for older versions.

MB: If we have an array, and the first is ESM and the second is a CJS, and you have two parts in your tree. One imports the ESM then CJS and the CJS uses the same specifier?

JK: Valid URLs will be taken. If it does not exist, it throws.

MB: It fails based on invalid URLs.

Guy: We could say only standard modules go through existence checks.

MB: So main cannot be used for dual modules. It will just fail in the require loader.

JK: Resolution is 100% repeatable.

MB: I like this idea. We're not deprecating, but it shadows it. In a Node that supports exports, it's used. And "main" is the backwards compatibility hook. So for Wes/Jordan, this could be the solution we talk about for older version of Node. So you can transpile ES5/ES3 and use it as main.

JK: The danger is that you have a package working on Node 10, then you upgrade and suddenly your app breaks because you were using require(). Having both fields makes this dangerous.

MB: I will chew on it. Are there concerns if we open a PR?

JS: This is a complicated discussion.

GB: This would be a great feature. The upgrade process is tough. It would be a huge win to have environment-specific fields.

MB: Hazard could be solved by best practice, e.g. console.log("you are using a legacy interface")

GB: Downgrading the dependency would solve this.

MB: We could have a mode:legacy that ignores exports and just uses "main". An opt-out could be sufficient to solve this.

Guy: I think it could be worthwhile bikeshedding a name for this.

JK: Yes. For the key name and the key location. I like the string|array possibility.

MB: Agreed. Enables simplicity.

  • Loader Hooks #351
  • Package Exports #341
  • Proposal: Support loading package by own "name" #306