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

vm: SourceTextModule refactor #29776

Closed
wants to merge 1 commit into from
Closed

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Sep 30, 2019

  • Removes redundant instantiate method
  • Refactors link to match the spec linking steps more accurately
  • Removes URL validation from SourceTextModule specifiers
  • DRYs some dynamic import logic

Closes #29030

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

@devsnek devsnek added the vm Issues and PRs related to the vm subsystem. label Sep 30, 2019
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 30, 2019
@devsnek
Copy link
Member Author

devsnek commented Sep 30, 2019

cc @nodejs/vm

@devsnek devsnek removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 30, 2019
doc/api/vm.md Outdated Show resolved Hide resolved
lib/internal/vm/source_text_module.js Show resolved Hide resolved
@devsnek devsnek added the experimental Issues and PRs related to experimental features. label Sep 30, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great to see these refinements.

doc/api/vm.md Outdated Show resolved Hide resolved
lib/internal/vm/source_text_module.js Show resolved Hide resolved
lib/internal/errors.js Show resolved Hide resolved
this.#error;
} catch {
throw new ERR_VM_MODULE_NOT_MODULE();
}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to abstract this into a private function for less repetition. That way it could be used as: this.#getPrivateProperty(name) where name would e.g., be '#error'.

Copy link
Member Author

@devsnek devsnek Oct 1, 2019

Choose a reason for hiding this comment

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

obj['#error'] is not the same as obj.#error, also you have to check for this.#getPrivateProperty in the first place 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Uh, of course! Too bad that it seems like there's no nice way around this 😞

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty ugly but it should be possible to abstract it like this:

function getPrivateProperty(module, name) {
  try {
    switch (name) {
      case 'error':
        return module.#error;
      ...
    }
  } catch {
    throw new ERR_VM_MODULE_NOT_MODULE();
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think how the current code is the best we can get it. you can blame tc39 for not generalizing private fields better 🤷

- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic

Closes nodejs#29030

Co-Authored-By: Michaël Zasso <targos@protonmail.com>
@nodejs-github-bot
Copy link
Collaborator

@devsnek devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 2, 2019
Trott pushed a commit that referenced this pull request Oct 2, 2019
- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic

Closes: #29030

Co-Authored-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #29776
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@Trott
Copy link
Member

Trott commented Oct 2, 2019

Landed in 5e7946f

@Trott Trott closed this Oct 2, 2019
@devsnek devsnek deleted the vm-module-2 branch October 2, 2019 23:50
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic

Closes: #29030

Co-Authored-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #29776
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. experimental Issues and PRs related to experimental features. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs vm.SourceTextModule: Instantiate() abstract operation has been replaced with InitializeEnvironment()
8 participants