Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP V8 API usage in Node.js #26929
WIP V8 API usage in Node.js #26929
Changes from 1 commit
7a63b32
5faa4c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space before
Local
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
.As<>
to convert e.g. functions to strings isn’t a valid thing to do, and I don’t think we should document it – that should crash in debug mode?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this I didn't understand. A
Function
becoming the source code of it's definition doesn't seem to be at all similar to a C-style untype-checked cast, unless a Function object is somehow derived from String. It appeared to me, while experimenting, to be doing the equivalent ofString(a_function)
-- so, not a cast.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sam-github Could you maybe give an example of how you came to that conclusion? It might help to know that. (If you used
String::Utf8Value
: That just “happens” to work out because the constructor of that class takes aLocal<Value>
, and explicitly converts the argument to string if necessary)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify, As should only be known when you are clarifying the handle type, not to perform a conversion. like Anna said, using As to perform an invalid cast will even abort in debug mode.
Boolean to number would be b->ToNumber(), number to string would be b->ToString(), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sam, can you remove these three bullet points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure which
To
you’re referring to here, but if you’re talking aboutToObject()
,ToBoolean()
, etc., then that’s different from.As<>()
–.As<>
performs a pointer cast without checking, but theTo...
methods perform the JS conversion operations, e.g.foo->ToNumber()
is the same as+foo
in JS.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That text describes what I saw when I wrote some scratch code to explore the difference: sam-github@3efb492 , so the behaviour I saw doesn't really agree with the As() is just a cast. I saw conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sam-github That should crash hard in debug mode as invalid casts – it’s just luck that V8 doesn’t crash in release mode, because the implementations of the methods you’re using there don’t actually require an object of the specified type. E.g.
Boolean::Value()
is really just checking whether the object strict-equalstrue
– that operation can work on any type.Number::Value()
uses a V8 internal that’s implemented for all object types, so it also just happens to work out by accident (possibly incorrectly – I don’t think it actually handles conversions).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this paragraph? There's no templated
To<T>
.(There's
Maybe<T>::To()
but that's different.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document
args.Holder()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that’s about it.
One kind-of-open question is whether we want Node’s own APIs to eventually supports multiple contexts, but that would be a major change to some of our internals…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does vm (or anything else) create new Contexts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the vm module does that. It’s currently the only public API for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… which is why this should be the same as
env->context()
in almost all cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinction isn’t that important to us because we don’t expose many APIs for more than one context (
MessagePort
being the only exception I can think of right now) – essentially, aFunctionTemplate
can be used to create functionally identical function instances for multiple contexts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionTemplates are also useful because they give you an ObjectTemplate (through InstanceTemplate()) that you can use to define internal fields. You can't do that with regular Functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy :-) ThrowErrnoException() and ThrowUVException() are old, the others are newer. I'd be okay with renaming them if that clears up the confusion / cognitive dissonance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be the same value that
module
has in CJS scripts – I think this is mostly there so that it matches the addon API more closely? We can remove it if we want, I’d say.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think anybody uses this in practice, neither Node.js itself nor addons. I think the idea was for it to function as sort of a opaque pointer, but that never became useful due to the way that we load addons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay to return empty handles, just not handles that point to something without going through
EscapableHandleScope::Escape()
.