FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: use InstantiateModule instead of deprecated by danbev · Pull Request #15423 · nodejs/node · GitHub

/ node Public

src: use InstantiateModule instead of deprecated - #15423

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:module_wrap_instantiate_deprecation_warning
ConversationCommits (1)ChecksFiles changed
Closed

src: use InstantiateModule instead of deprecated#15423
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:module_wrap_instantiate_deprecation_warning

Conversation

danbev commented Sep 15, 2017

Copy link
Copy Markdown
Contributor

The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2017
targos reviewed Sep 15, 2017
View reviewed changes
Comment thread src/module_wrap.cc Outdated

targos Sep 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok.FromMaybe(false)

danbev Sep 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice, I was not aware of that function. Thanks.

src: use InstantiateModule instead of deprecated
3ebf050 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.
danbev force-pushed the module_wrap_instantiate_deprecation_warning branch from 51bc74b to 3ebf050 Compare September 15, 2017 09:24

danbev commented Sep 15, 2017

Copy link
Copy Markdown
Contributor Author

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

mscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 15, 2017

targos commented Sep 18, 2017

Copy link
Copy Markdown
Member

Landed in bd85752

targos closed this Sep 18, 2017
targos pushed a commit that referenced this pull request Sep 18, 2017
src: use InstantiateModule instead of deprecated
bd85752 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: #15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell commented Sep 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

If I understand this correctly, this depends on v8 6.1 correct? So it cannot be backported to v8.x until 8.1 lands. Ping @MylesBorins so this is on the radar for when v8 6.1 lands.

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
src: use InstantiateModule instead of deprecated
4a996d5 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: nodejs/node#15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
src: use InstantiateModule instead of deprecated
4dde216 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: nodejs/node#15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
src: use InstantiateModule instead of deprecated
9db22eb 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: #15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
src: use InstantiateModule instead of deprecated
e6b64fa 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: #15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
src: use InstantiateModule instead of deprecated
902feea 
The following deprecation warning is displayed when compiling:

../src/module_wrap.cc:187:18: warning: 'Instantiate' is deprecated
[-Wdeprecated-declarations]
  bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);
                 ^
../deps/v8/include/v8.h:1158:22: note: 'Instantiate' has been explicitly
marked deprecated here
                bool Instantiate(Local<Context> context,
                     ^

This commit changes this function call to use InstantiateModule instead
which returns a Maybe<bool>.

PR-URL: #15423
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
danbev deleted the module_wrap_instantiate_deprecation_warning branch November 16, 2017 08:35
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
fhinkel fhinkel approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL