Skip to content

introduce loader hook context - #20761

Closed
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:feature/esm-hook-context
Closed

introduce loader hook context#20761
devsnek wants to merge 2 commits into
nodejs:masterfrom
devsnek:feature/esm-hook-context

Conversation

@devsnek

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label May 16, 2018
@devsnekdevsnek added the esm Issues and PRs related to the ECMAScript Modules implementation. label May 16, 2018
@devsnek
devsnekforce-pushed the feature/esm-hook-context branch from dab4ed2 to e93d419CompareMay 16, 2018 01:17
@CedarXi

Copy link
Copy Markdown

good

Comment threaddoc/api/esm.md
useful if resolve is not hooked.
- `specifier` {string} The specifier of the module to import.
- `parentURL` {string} The URL of the module that requested the specifier.
- `vmModuleLinkHook` {object} This value can be passed to [`module.link`][] to

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

`module.link` -> `module.link()`?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

{object} -> {Object}.

Comment threaddoc/api/esm.md
then be called at the exact point of module evaluation order for that module
in the import tree.

[`module.link`]: vm.html#vm_module_link_linker

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

`module.link` -> `module.link()`?

const k = Object.freeze(Object.create(null));
vmModuleLinkHookMap.set(k, this);

this.hookContext = Object.assign(Object.create(null), {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer a different name since this has relations to vm and Context has a meaning of a Realm in the vm terminology.

This also seems to have some conflict with #18914 that I need to think on.

const linkingFromLoader = vmModuleLinkHookMap.has(linker);
if (linkingFromLoader) {
const loader = vmModuleLinkHookMap.get(linker);
linker = (specifier, parent) =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reason not to use an async function here?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

habits are hard to break :) nice catch

defaultResolve,
resolve: (specifier, parentURL) => this.resolve(specifier, parentURL),
vmModuleLinkHook: k,
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to deny the hook context access to outside world, you are not – the [[Prototype]] of these functions are the %FunctionPrototype% of the outside world. It would be better to create these functions in the context directly, if possible, rather than using Object/Reflect.setPrototypeOf().

@devsnek

Copy link
Copy Markdown
MemberAuthor

I came up with an alternative approach to this which should be a bit nicer

@devsnekdevsnek closed this May 18, 2018
@devsnek
devsnek deleted the feature/esm-hook-context branch May 18, 2018 15:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

esmIssues and PRs related to the ECMAScript Modules implementation.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@devsnek@CedarXi@bmeck@TimothyGu@vsemozhetbyt@nodejs-github-bot