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

convert NativeMath and NativeJSON into a Lambda based ScriptableObject #1384

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Aug 30, 2023

and add the toStringTag property

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 1, 2023

Given that it unbreaks a test in test262 and that IMHO the new code is MUCH easier to reason with, I'm all for this change

Are there any other upsides (performance?) or maybe some downsides to using a Lambda-based impl. vs. the IdScriptableObject approach?

@rbri
Copy link
Collaborator Author

rbri commented Sep 1, 2023

For me the impl of the toStringTag property based on lambda was simpler and it was just a test to see how hard it might be to do the same for RegEx. This is the next on my list (but after the vacation in october).

Regarding positive/negative side effects i guess @gbrail knows more.

But overall for me having this lambdas opens up some great improvements, its much simpler and more straight forward to implement the stuff in this way.

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 1, 2023

But overall for me having this lambdas opens up some great improvements, its much simpler and more straight forward to implement the stuff in this way.

Yeah, it looks sooo much cleaner :-)

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 1, 2023

Any idea yet whether the Lambda approach will make #963 / #1181 (comment) easier?

@gbrail
Copy link
Collaborator

gbrail commented Sep 1, 2023

Yes, I'm happy to see this too. I actually don't think that there are big downsides to the lambda approach -- I also found native JS stuff to be easier to implement that way as it maps much more directly to the native way that JavaScript works, and furthermore I've tested the performance and I don't recall seeing a significant difference.

One side note on all this -- it'd be easier for me if we start marking PRs "draft" if you are doing it just for discussion, or want input, and un-marking them "draft" when you want them to be seriously considered for merging.

For this particular case, I'm happy to merge this unless there are other concerns.

@rbri
Copy link
Collaborator Author

rbri commented Sep 2, 2023

i like to see it merged because it fixes two more test

@gbrail
Copy link
Collaborator

gbrail commented Sep 9, 2023

I think this works and makes sense. Thanks!

@gbrail gbrail merged commit 1485f1e into mozilla:master Sep 9, 2023
3 checks passed
@rbri rbri deleted the lambda_math branch April 15, 2024 16:02
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

3 participants