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

src: use InstantiateModule instead of deprecated #15423

Closed
wants to merge 1 commit into from
Closed

src: use InstantiateModule instead of deprecated #15423

wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Sep 15, 2017

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

// clear resolve cache on instantiate
obj->resolve_cache_.clear();

if (!ok) {
if (!ok.ToChecked()) {
Copy link
Member

Choose a reason for hiding this comment

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

ok.FromMaybe(false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@danbev
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 danbev force-pushed the module_wrap_instantiate_deprecation_warning branch from 51bc74b to 3ebf050 Compare September 15, 2017 09:24
@danbev
Copy link
Contributor Author

danbev commented Sep 15, 2017

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

@mscdex mscdex added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 15, 2017
@targos
Copy link
Member

targos commented Sep 18, 2017

Landed in bd85752

@targos targos closed this Sep 18, 2017
targos pushed a commit that referenced this pull request Sep 18, 2017
@danbev @targos
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 20, 2017 •
edited
Loading

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
@danbev
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@danbev
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
@danbev @MylesBorins
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
@danbev @MylesBorins
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@danbev @MylesBorins
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@danbev danbev deleted the module_wrap_instantiate_deprecation_warning branch November 16, 2017 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@fhinkel fhinkel fhinkel approved these changes

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
@danbev @targos @jasnell @fhinkel @cjihrig @mscdex @nodejs-github-bot