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.

Sign up for GitHub

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

Jump to bottom

v8 Synthetic Modules Implementation #29846

Closed
wants to merge 2 commits into from
Closed

v8 Synthetic Modules Implementation #29846

wants to merge 2 commits into from

Conversation

guybedford
Copy link
Contributor

This updates uses of createDynamicModule to instead use a new ModuleWrap signature for creating v8 Synthetic Module Records instead.

We still need to retain createDynamicModule for WASM due to imports and for the dynamic loader due to the exact behaviours, but these uses will likely be phased out over time anyway.

This PR is based on top of #29737, so should merge after that. This is also prerequisite to the unflagging, as discussed in nodejs/modules#394.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 4, 2019
@guybedford
Copy link
Contributor Author

Relevant commit for review - cb2654c

devsnek
devsnek suggested changes Oct 4, 2019
View reviewed changes
src/module_wrap.h Show resolved Hide resolved
src/module_wrap.cc Show resolved Hide resolved
@guybedford guybedford mentioned this pull request Oct 4, 2019
Closed
4 tasks
@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 5, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25858/

@devsnek
Copy link
Member

devsnek commented Oct 5, 2019

i'm pretty sure that REPLACEME is supposed to be a REPLACEME

@guybedford
Copy link
Contributor Author

@devsnek you mean in that it's intended to be left in or replaced!?

@devsnek
Copy link
Member

devsnek commented Oct 5, 2019

@guybedford you replaced an example showing how to add a new method to the docs.

@guybedford
Copy link
Contributor Author

Ok I've rebased to the correct fix for the REPLACEME from #29737.

@guybedford guybedford mentioned this pull request Oct 6, 2019
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25875/

@devsnek devsnek added esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. v8 engine Issues and PRs related to the V8 dependency. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 6, 2019
@devsnek
Copy link
Member

devsnek commented Oct 6, 2019

@guybedford what's up with that fixup eval commit? is node currently failing without it?

@guybedford
Copy link
Contributor Author

@devsnek no Node master is fine actually. It was just a bug fix on this PR.

@devsnek
Copy link
Member

devsnek commented Oct 6, 2019

@guybedford oh cool, just wanted to make sure it wasn't going in as a separate commit :)

(for future reference, you can prefix commits with squash! or fixup! to let git and other humans know that they're meant to be part of previous commits when rebasing)

@guybedford
Copy link
Contributor Author

@Trott are we good to merge this later today? Or do we still do 72 hours for weekends? (I couldn't find reference to this in the collaborating guide anymore though)

@devsnek
Copy link
Member

devsnek commented Oct 6, 2019

according to NCU this needs to wait one more hour

guybedford added a commit that referenced this pull request Oct 6, 2019
@guybedford
module: use v8 synthetic modules
ffd22e8 
PR-URL: #29846
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@guybedford
Copy link
Contributor Author

Landed in ffd22e8.

@guybedford guybedford closed this Oct 6, 2019
@guybedford guybedford deleted the dynamic-synthetic branch October 6, 2019 17:45
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Oct 7, 2019
Merged
4 tasks
@Trott
Copy link
Member

Trott commented Oct 8, 2019

@Trott are we good to merge this later today? Or do we still do 72 hours for weekends? (I couldn't find reference to this in the collaborating guide anymore though)

@guybedford Yes, we got rid of the 72-hour rule. It's always 48 hours now.

BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
@guybedford @BridgeAR
module: use v8 synthetic modules
90562ae 
PR-URL: #29846
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@JungMinu JungMinu JungMinu approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@guybedford @nodejs-github-bot @devsnek @Trott @JungMinu