fix(compartment-mapper): Missing entries and conflated namespace #1671
Conversation
|
cc @Chris-Hibbert @warner, fix for #1206 incoming. |
| @@ -71,7 +69,6 @@ const sortedModules = ( | |||
| } | |||
| seen.add(key); | |||
|
|
|||
| const resolve = compartmentResolvers[compartmentName]; | |||
There was a problem hiding this comment.
FWIW I understand this and consider it an improvement.
| ); | ||
| entryCompartmentName, | ||
| entryModuleSpecifier, | ||
| }); |
There was a problem hiding this comment.
The long overdue switch to an options bag.
| /** @type {Map<string, Set<string>>} compartment name ->* module specifier */ | ||
| const strictlyRequired = new Map([ | ||
| [entryCompartmentName, new Set([entryModuleSpecifier])], | ||
| ]); |
There was a problem hiding this comment.
Why does strictlyRequired need to depend on a compartment? If a module is strictly required at least once in the entire compartment-map, it should not be deferred.
Why was putting the entryModuleSpecifier in the set not enough?
Why would it be ok to defer a module in one compartment while it's strictlyRequired in another compartment?
I expect to find out when I reach the new tests, but the only reason I can think of right now is if the specifier we're using as a set entry could point to different things in different compartments, in which case we probably should look for a different thing to identify the package with?
On the other hand, whether this is per compartment or not doesn't affect the end result - the one compatment where ESM import makes a specifier strictly required will be enough to cause an error to be thrown instead of the bundle/archive being created.
It might take a few more operations to get there though. With a flat set if the first attempt to get the strictly required module was a cjs require, it would also throw AFAIR.
I'll get back to this comment when I finish. This is how far I got for now. Back in a few hours.
There was a problem hiding this comment.
Module specifiers are scoped to compartment. For example, ./index.js refers to a different module in compartment A and compartment B.
There was a problem hiding this comment.
Yeah, got it. So we could fall back to path, but that would be more difficult and less performant than this.
kriskowal
force-pushed
the
kris-missing-entry
branch
from
7fdb843 to
b7687e9
Compare
…tween compartments
kriskowal
force-pushed
the
kris-missing-entry
branch
from
b7687e9 to
b304a88
Compare
This change includes related fixes in the compartment mapper and a drive-by refactor.
In order to allow CommonJS to conditionally import a dependency, it must be possible for that dependency to be absent. To that end, the Compartment Mapper tracks which modules are strictly required (because they’re retained by an ESM, as opposed to CJS).
Fixes #1206:
bundleSourcegenerates invalid bundles if the entry module does not exist, where it should simply fail to generate a bundle. The fix is to include the entry module in the set of strictly required modules.This also fixes overlapping bugs I missed in review:
Each compartment must have a separate set of strictly required module specifiers. This change includes a test that illustrates that a compartment map may include two packages that have the same module name, but retain that module weakly in one but strictly in the other. This test failed before the fix.
The entries in the strictly required sets must also be resolved. A single package can use the same import specifier to refer to different full module specifiers, retaining one but not the other. Teasing out a failing test for this was a bit more tricky. The test relies on a dependency upon
../inconsistent.jsbeing strictly retained from ESM and weakly retained from a different CJS. In order to cause the collision, such that the relative import specifiers collided but the full module specifiers did not, it was necessary for the importing modules to have different directory depths.As for a drive-by: currently, we only support Node.js-style resolve hooks. If we supported other forms, they would have to be mentioned by name in the compartment map. Since we don’t, we can pretend the compartment maps have an invisible
"resolve": "node"and that this would be the default going forward. Since we don’t have other resolution algorithms, nothing is achieved in parameterizing the Compartment Mapper. In all the places we would have consulted a table of resolver hooks for each package, we also have direct access to the compartment map, so we would use that instead.