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

[Proposal] Add unsafe convenience methods for JSValue #98

Merged
merged 2 commits into from Oct 15, 2020

Conversation

kateinoigakukun
Copy link
Member

I got feedback that the current API to access JavaScript objects is redundant.
As he says, I agree that method!(...) and .object! for every object access is very redundant. I think we can provide unsafe convenience methods with some assumptions.

Before

let document = JSObject.global.document.object!
let foundDivs = document.getElementsByTagName!("div").object!

After

let document = JSObject.global.document
let foundDivs = document.getElementsByTagName("div")

What do you think about this proposal?

@github-actions
Copy link

Time Change: +195ms (2%)

Total Time: 9,409.5ms

Test name Duration Change
Serialization/Write JavaScript number directly 186.75ms +15.5ms (8%) 🔍
ℹ️ View Unchanged
Test name Duration Change
Serialization/Write JavaScript string directly 184ms +8ms (4%)
Serialization/Swift Int to JavaScript 3,088.25ms +100ms (3%)
Serialization/Swift String to JavaScript 3,119.75ms +71.75ms (2%)
Object heap/Increment and decrement RC 2,830.75ms -0.25ms

performance-action

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

I love it, and I wasn't sure why we didn't have it this way from the start. I always thought there was something in how dynamicMember works that prevented this from working.

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

I’m a little worried that it doesn’t have an unsafe prefix — maybe we could have these return optionals instead?

This doesn’t hurt the usability very much and clarifies that you’re doing something unsafe:

let document = JSObject.global.document
let foundDivs = document.getElementsByTagName!("div")

Sources/JavaScriptKit/JSValue.swift Show resolved Hide resolved
@kateinoigakukun
Copy link
Member Author

The only thing I concerned about this change was overloads ambiguity, but it looks working well in Tokamak usage while my testing.

@kateinoigakukun
Copy link
Member Author

Can we merge this?

@MaxDesiatov MaxDesiatov requested review from j-f1 and a team October 14, 2020 13:09
@kateinoigakukun kateinoigakukun merged commit 06a828e into main Oct 15, 2020
@kateinoigakukun kateinoigakukun deleted the katei/jsvalue-convenience-methods branch October 15, 2020 03:31
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.

None yet

5 participants