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

V8: upcoming deprecation warnings #18909

Closed
5 tasks
targos opened this issue Feb 21, 2018 · 11 comments
Closed
5 tasks

V8: upcoming deprecation warnings #18909

targos opened this issue Feb 21, 2018 · 11 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Feb 21, 2018

The latest canary update uncovered a few new deprecations from V8.
I didn't check, but those can probably already be fixed on master.

  • v8::Message::GetLineNumber() in node.cc:

    • To be replaced with Maybe<int> GetLineNumber(Local<Context> context)
    ../src/node.cc:1273:40: warning: ‘int v8::Message::GetLineNumber() const’ is deprecated: 
    Use maybe version [-Wdeprecated-declarations]
       int linenum = message->GetLineNumber();
                                            ^
    
  • v8::Message::GetSourceLine() in node.cc:

    • To be replaced with MaybeLocal<String> GetSourceLine(Local<Context> context)
    ../src/node.cc:1275:69: warning: ‘v8::Local<v8::String> v8::Message::GetSourceLine() const’ is deprecated: Use maybe version [-Wdeprecated-declarations]
       node::Utf8Value sourceline(env->isolate(), message->GetSourceLine());
                                                                         ^
    
  • v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>) in node.cc, node_api.cc, node_buffer.cc, node_v8.cc and node_crypto.cc:

    • To be replaced with Utf8Value(Isolate* isolate, Local<v8::Value> obj)
    ../src/node.cc:1423:35: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
           String::Utf8Value message(er);
                                       ^
    
    ../src/node_api.cc:3323:63: warning: 
    ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
                         *v8::String::Utf8Value(async_resource_name)),
                                                                   ^
    
    ../src/node_buffer.cc:990:42: warning: 
    ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
         String::Utf8Value needle_value(needle);
                                              ^
    
    ../src/node_v8.cc:117:34: warning: ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
       String::Utf8Value flags(args[0]);
                                      ^
    
    ../src/node_crypto.cc:4176:21: warning: 
    ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
       String::Utf8Value passphrase(args[3]);
                         ^~~~~~~~~~
    
  • v8::Script::Run() in node.cc and node_contextify.cc:

    • To be replaced with MaybeLocal<Value> Run(Local<Context> context)
    ../src/node.cc:1475:54: warning: ‘v8::Local<v8::Value> v8::Script::Run()’ is deprecated: Use maybe version [-Wdeprecated-declarations]
       Local<Value> result = script.ToLocalChecked()->Run();
                                                          ^
    
    ../src/node_contextify.cc:1079:28: warning: ‘v8::Local<v8::Value> v8::Script::Run()’ is deprecated: Use maybe version [-Wdeprecated-declarations]
           result = script->Run();
                                ^
    
    ../src/node_contextify.cc:1082:28: warning: ‘v8::Local<v8::Value> v8::Script::Run()’ is deprecated: Use maybe version [-Wdeprecated-declarations]
           result = script->Run();
                                ^
    
    ../src/node_contextify.cc:1085:28: warning: ‘v8::Local<v8::Value> v8::Script::Run()’ is deprecated: Use maybe version [-Wdeprecated-declarations]
           result = script->Run();
                                ^
    
    ../src/node_contextify.cc:1087:28: warning: ‘v8::Local<v8::Value> v8::Script::Run()’ is deprecated: Use maybe version [-Wdeprecated-declarations]
           result = script->Run();
                                ^
    
  • v8::String::Value::Value(v8::Local<v8::Value>) in node_buffer.cc, string_bytes.cc and inspector_js_api.cc:

    • To be replaced with Value(Isolate* isolate, Local<v8::Value> obj)
    ../src/node_buffer.cc:957:38: warning: ‘v8::String::Value::Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
         String::Value needle_value(needle);
                                          ^
    
    ../src/string_bytes.cc:372:32: warning: ‘v8::String::Value::Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
             String::Value value(str);
                                    ^
    
    ../src/string_bytes.cc:383:32: warning: ‘v8::String::Value::Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
             String::Value value(str);
                                    ^
    
    ../src/string_bytes.cc:482:30: warning: ‘v8::String::Value::Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
           String::Value value(str);
                                  ^
    
    ../src/inspector_js_api.cc:241:42: warning: ‘v8::String::Value::Value(v8::Local<v8::Value>)’ is deprecated: Use Isolate version [-Wdeprecated-declarations]
       String::Value task_name_value(task_name);
                                              ^
    
@targos targos added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. good first issue Issues that are suitable for first-time contributors. labels Feb 21, 2018
@targos
Copy link
Member Author

targos commented Feb 21, 2018

Edit: Alternative APIs are all available on master. I added the signatures of the methods that are supposed to be used in the OP.

@hashseed
Copy link
Member

We are actually actively working on moving stuff marked as V8_DEPRECATE_SOON to V8_DEPRECATED.

@targos
Copy link
Member Author

targos commented Feb 21, 2018

@hashseed good to know! I'll update this issue with the new deprecations as they arrive.

@hashseed
Copy link
Member

This is the tracking bug for that effort.

@inidaname
Copy link

If this is still open may I know if I can take it up.

@gibfahn
Copy link
Member

gibfahn commented Feb 21, 2018

If this is still open may I know if I can take it up.

Go for it. If you have issues you can comment here.

@helmutgranda
Copy link

Are those listed above 1:1 replacements?

From:
v8::Message::GetLineNumber()
To:
v8::Message::Maybe GetLineNumber(Local context)

@joyeecheung
Copy link
Member

joyeecheung commented Feb 22, 2018

@helmutgranda They should be.

FWIW I have put up an initial version about how to migrate the deprecated APIs in the C++ style guide quite some time ago, just have not found the time to polish it and open a PR, but here it is:

https://github.com/joyeecheung/node/blob/v8-maybe-doc/CPP_STYLE_GUIDE.md#use-maybe-version-of-v8-apis

Since there are some maybes that should be handled with care (proper cleanups), I am not quite sure how many of them are actually good first contributions (In my understanding good first contributions are not PRs that potentially need more than 3 rounds of reviews, those should be uh..probably good second contributions or something for people who are already familiar with the PR process).

@wuweiweiwu
Copy link
Contributor

@targos @gibfahn Can I pick this up? I think it'll be a good first issue :)

@inidaname
Copy link

@wuweiweiwu if agreed please feel free to lately I've been held up with some events preparation.

@SirR4T
Copy link
Contributor

SirR4T commented Mar 21, 2018

@joyeecheung / @targos are tests necessary for these changes? If so, how to go about writing them?

SirR4T added a commit to SirR4T/node that referenced this issue Mar 23, 2018
@targos targos closed this as completed in c6c957d Mar 23, 2018
targos pushed a commit that referenced this issue Mar 30, 2018
PR-URL: #19490
Fixes: #18909
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
blattersturm pushed a commit to citizenfx/node that referenced this issue Nov 3, 2018
PR-URL: nodejs#19490
Fixes: nodejs#18909
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
manueltimita added a commit to manueltimita/node-shared-cache that referenced this issue Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants