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

JSG Completion: add interim jsg::Lock::v8Set and jsg::Lock::v8Get #971

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 4, 2023

Help to eliminate v8 specific boilerplate when getting/setting values on v8::Object instances. This should be considered transitional. It helps to eliminate a number of direct uses of v8::Isolate and other v8 specific APIs but does not completely hide the v8 semantics.

For example,

  jsg::check(bound->Set(js.v8Context(), jsg::v8StrIntern(js.v8Isolate, "foo"_kj), handler.wrap(js, JSG_THIS)));

  // becomes

  js.v8Set(bound, "asyncResource"_kj, handler.wrap(js, JSG_THIS));

@kentonv
Copy link
Member

kentonv commented Aug 4, 2023

Is the idea that eventually we'll have a wrapper for v8::Local<v8::Object> that has get() and set() methods?

I guess v8Set() and v8Get() have the funny-looking v8 prefix because they operate on v8::Local rather than a JSG wrapper type?

@jasnell
Copy link
Member Author

jasnell commented Aug 4, 2023

Is the idea that eventually we'll have a wrapper for v8::Local<v8::Object> ...

I think so but still thinking through it.

I guess v8Set() and v8Get() have the funny-looking v8 prefix ...

Yep, that's exactly it.

@jasnell jasnell force-pushed the jsnell/jsg-completion-v8setget branch 2 times, most recently from a65a73e to 49bcb9a Compare August 4, 2023 17:56
@jasnell jasnell marked this pull request as draft August 4, 2023 18:34
@jasnell

This comment was marked as resolved.

@jasnell jasnell force-pushed the jsnell/jsg-completion-v8setget branch 3 times, most recently from 00bae72 to c401d58 Compare August 4, 2023 21:15
@jasnell jasnell marked this pull request as ready for review August 4, 2023 21:16
src/workerd/jsg/jsg.h Outdated Show resolved Hide resolved
Copy link
Member

@kentonv kentonv left a comment

Choose a reason for hiding this comment

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

Approved assuming v8Set can be changed to return void.

@jasnell jasnell force-pushed the jsnell/jsg-completion-v8setget branch from c401d58 to a3bd2fc Compare August 7, 2023 18:15
Help to eliminate v8 specific boilerplate when getting/setting values
on v8::Object instances. This should be considered transitional. It
helps to eliminate a number of direct uses of v8::Isolate and other
v8 specific APIs.
@jasnell jasnell force-pushed the jsnell/jsg-completion-v8setget branch from a3bd2fc to b7939f9 Compare August 7, 2023 18:35
@jasnell jasnell merged commit 2c5b59e into main Aug 7, 2023
7 checks passed
@fhanau fhanau deleted the jsnell/jsg-completion-v8setget branch December 4, 2023 22:32
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

Successfully merging this pull request may close these issues.

2 participants