Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(endo): Add assemble #345

Merged
merged 2 commits into from
Jul 21, 2020
Merged

feat(endo): Add assemble #345

merged 2 commits into from
Jul 21, 2020

Conversation

kriskowal
Copy link
Member

The compartment assembler takes a compartment map and builds the
corresponding compartment instances, feeding the modules exported by
each dependency into the dependee compartment's module map.

@kriskowal kriskowal requested a review from warner June 18, 2020 22:04
@kriskowal kriskowal mentioned this pull request Jun 19, 2020
36 tasks
@kriskowal kriskowal force-pushed the kris/endo-assemble branch 2 times, most recently from a71cc4d to 84fd542 Compare June 19, 2020 19:08
@kriskowal kriskowal force-pushed the kris/endo-assemble branch 2 times, most recently from 7031ca8 to 6af7973 Compare June 19, 2020 20:21
@kriskowal kriskowal force-pushed the kris/endo-assemble branch 2 times, most recently from 1dea943 to 6e5f172 Compare June 24, 2020 20:47
@kriskowal kriskowal force-pushed the kris/endo-search branch 3 times, most recently from 37bbe67 to fb56681 Compare June 30, 2020 20:12
@warner
Copy link
Contributor

warner commented Jul 20, 2020

I'm guessing this branch pointer needs to be reset to something descending from endo-search? I'll study the one file (assemble.js) as it is in this branch for now. (I see that the file is the same, so hopefully the rebase should be easy).

Copy link
Contributor

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only looked at assemble.js, because I'm assuming this branch needs to be re-sewn into the main sequence of PRs (and the other files are covered by other PRs), but it looks good to me.

// Passes the given endowments and external modules into the root compartment
// only.
export const assemble = ({
name,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is the same as packageLocation in compartmap.js, right? And it's not the label from that data structure. And I think it's nominally a URL (although compartments[name].root, which happens to be a copy of name, is probably more deserving of an identifier which has "location" in the name since I think that .root will be used to actually find the code).

If the key of compartments is not a load-bearing, I mean, location-bearing identifier, then name is a great parameter name to use. But maybe there's something we could to in compartmap.js to make that clear. Not sure what, maybe:

const translateGraph = (mainPackagePath, graph) => {
  const compartments = {};

  for (const [packageLocation, { label, dependencies }] of entries(graph)) {
  ...
  const name = packageLocation; // not used for location properties, just a distinct name for each compartment
  ...
    compartments[name] = {
      label, // cosmetic, 'packagename@version'
      root: packageLocation, // used to find code for the package

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The for purposes of assembling compartments, the names are just internal references and are only obliged to be internally consistent. In practice, if the compartment map is prepared by importLocation, they are locations; if the compartment map is prepared by writeArchive, they are unique identifiers. Both of these mechanisms are free to change independent of the assembler.

I can probably make that more clear in code as you’re suggesting.

throw new Error(`Cannot assemble compartment graph that includes a cycle`);
}

for (const [inner, outer] of entries(descriptor.modules || {})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the fallback? compartmap.js always provides a .module property, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but left this affordance against the possibility that compartmap.json might be manually manipulated, or that section automatically omitted if empty.

loaded,
Compartment
});
modules[inner] = compartment.module(moduleSpecifier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial caller can pass modules which will be made available to the top-level Compartment, in addition to modules for all of its dependencies (specifically all of the exported modules for all of its dependency packages). Cool. But the dependency modules will shadow any provided by the caller, so this feature couldn't be used for e.g. dependency injection. Is that intentional? I can imagine there's a good reason to do it one way or the other, or maybe even both, but I don't know what those reasons might be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent is for this to be usable for dependency injection. I’ll look more closely and will make sure it’s tested.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked. As written, this is fine for injecting built-in modules that are not declared dependencies, but not for replacing third-party dependencies with overrides.

I think I will land as-is and revisit. I think I want an explicit directive in the compartment map when a package should receive an override. The same mechanism would be used to thread built-in modules expressly to third-party packages and would be driven by policies in package.json or a lavamoat config file.

@kriskowal kriskowal force-pushed the kris/endo-assemble branch 2 times, most recently from 8bb4f7b to 0deea46 Compare July 21, 2020 20:14
Base automatically changed from kris/endo-search to master July 21, 2020 20:25
kriskowal and others added 2 commits July 21, 2020 13:25
The compartment assembler takes a compartment map and builds the
corresponding compartment instances, feeding the modules exported by
each dependency into the dependee compartment's module map.
@kriskowal kriskowal merged commit 7d71ef4 into master Jul 21, 2020
@kriskowal kriskowal deleted the kris/endo-assemble branch July 21, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants