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::StringObject::New(v8::Local<v8::String>&) is not available in latest v8-canary build #834

Closed
nolanmar511 opened this issue Dec 17, 2018 · 7 comments

Comments

@nolanmar511
Copy link
Contributor

nolanmar511 commented Dec 17, 2018

I see the deprecation warnings are silenced where v8::StringObject::New(v8::Local<v8::String>&) is used (here). With the latest v8-canary release, this method is no longer available.

It would be awesome if it were possible to use nan in (some capacities) with v8-canary build!

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

see #831

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 18, 2018

As mentioned in #831 it will not be merged quite yet, since more breaking changes might come, but if you check out that PR branch everything should work for now.

@nolanmar511
Copy link
Contributor Author

How long would it be until #831 would be merged in?

It's hard for me to both depend on NAN and ensure that my module won't break with the next release of Node.js given that my module (or any module that includes "nan.h") depending on NAN can't be compile with Node.js master.

While downloading the code from a PR is possible, I'm not super comfortable with downloading code from a PR in a continuous integration test for several months.

@nolanmar511
Copy link
Contributor Author

I should mention, the module I work on might be a bit weird. It uses v8's profiling tools, so we like have a continuous e2e test which uses the v8-canary build, to try to find any problems that might occur in the profilers we use sooner.

I know this use case isn't really supported by NAN, but it's definitely nice when it works.

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 18, 2018

I was planning on holding out until the release of Node 12 or just before. While I suppose there is no greater harm in merging it earlier, that would still require pulling the master branch in place of the #831 branch.

@kkoopa
Copy link
Collaborator

kkoopa commented Dec 18, 2018

Alright, it is merged.

@nolanmar511
Copy link
Contributor Author

Thanks!

@kkoopa kkoopa closed this as completed Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants