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

Remove V8 deprecation warnings #568

Merged
merged 3 commits into from
May 28, 2016
Merged

Remove V8 deprecation warnings #568

merged 3 commits into from
May 28, 2016

Conversation

matthewloring
Copy link
Contributor

Current deprecation warnings on Node v6 are SetAccessor, CloneElementAt,
and BooleanObject::New.

bnoordhuis
bnoordhuis reviewed May 9, 2016
View reviewed changes
nan_implementation_12_inl.h
@@ -37,7 +37,11 @@ Factory<v8::Boolean>::New(bool value) {

Factory<v8::BooleanObject>::return_t
Factory<v8::BooleanObject>::New(bool value) {
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
return v8::BooleanObject::New(v8::Isolate::GetCurrent(), value).As<v8::BooleanObject>();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@bnoordhuis
Copy link
Member

LGTM with a style nit.

@matthewloring
Copy link
Contributor Author

@bnoordhuis The CloneElementAt deprecation message says that "Cloning is not supported.". How should this be handled here as there is not an alternative that I am aware of.

@matthewloring matthewloring mentioned this pull request May 9, 2016
Closed
@kkoopa
Copy link
Collaborator

kkoopa commented May 9, 2016

What about WeakCallbackInfo::IsFirstPass?

@matthewloring
Copy link
Contributor Author

Hmm, I wasn't seeing a warning for that one. I found these warnings by compiling an arbitrary native module with warnings enabled. Is there an easy way to exercise the full API to make sure I've got all the deprecations? It looks like WeakCallbackInfo::IsFirstPass falls into the same category as CloneElementAt as having no clear alternative API. How do you recommend dealing with these cases?

@kkoopa
Copy link
Collaborator

kkoopa commented May 9, 2016

The test suite covers most, but it is far from complete. The removed functionality should be reimplemented without breaking the API. Functional equivalence with no unwanted side effects.

Cloning an array element should be fairly straightforward, as it only needs special logic for v8::Objects, although I do not know what all is cloned. The weak callback logic will require a fair amount of rewriting, since it appears to have been unreliable forever.

@matthewloring
Copy link
Contributor Author

@kkoopa I've got a reimplementation of CloneElementAt. Both Clone and CloneElementAt eventually call CopyJSObject so the should share copy semantics. IsFirstPass will require more work. It may be good to address that one in a larger follow on PR.

@kkoopa
Copy link
Collaborator

kkoopa commented May 10, 2016

Sounds good. IsFirstPass might be reworkable by adding a constant boolean template parameter to the function, essentially turning it into two functions.

On May 10, 2016 2:39:36 AM GMT+03:00, Matthew Loring notifications@github.com wrote:

@kkoopa I've got a reimplementation of CloneElementAt. Both Clone
and CloneElementAt eventually call CopyJSObject so the should share
copy semantics. IsFirstPass will require more work. It may be good to
address that one in a larger follow on PR.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#568 (comment)

@kkoopa kkoopa mentioned this pull request May 10, 2016
Merged
kkoopa
kkoopa reviewed May 13, 2016
View reviewed changes
nan_maybe_43_inl.h
@@ -217,7 +217,13 @@ NAN_INLINE Maybe<int> GetEndColumn(v8::Local<v8::Message> msg) {
NAN_INLINE MaybeLocal<v8::Object> CloneElementAt(
v8::Local<v8::Array> array
, uint32_t index) {
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
v8::EscapableHandleScope handle_scope(v8::Isolate::GetCurrent());
v8::Local<v8::Object> elem = array->Get(index)->ToObject()->Clone();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Remove V8 deprecation warnings
e509f76 
Current deprecation warnings on Node v6 are SetAccessor, CloneElementAt,
and BooleanObject::New.
kkoopa
kkoopa reviewed May 16, 2016
View reviewed changes
nan_maybe_43_inl.h
return array->CloneElementAt(GetCurrentContext(), index);
v8::Local<v8::Context> context = GetCurrentContext();
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
v8::EscapableHandleScope handle_scope(v8::Isolate::GetCurrent());

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@matthewloring
Copy link
Contributor Author

@kkoopa Do you want any further changes on this PR?

@kkoopa
Copy link
Collaborator

kkoopa commented May 23, 2016

You could escape the scope before returning on line 233, other than that it looks good to merge. However, I have been waiting for someone to review #569, since it also is necessary to avoid deprecation warnings.

@matthewloring
Copy link
Contributor Author

All fixed.

@kkoopa kkoopa merged commit 0592fb0 into nodejs:master May 28, 2016
@rchipka rchipka mentioned this pull request Jan 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewloring @bnoordhuis @kkoopa