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

Remove custom __getClass method #298

Merged
merged 1 commit into from Mar 10, 2024
Merged

Remove custom __getClass method #298

merged 1 commit into from Mar 10, 2024

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Mar 9, 2024

No description provided.

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

👍

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

Why not use a.constructor === RegExp or possibly a?.constructor === RegExp ?

@bnoordhuis
Copy link
Contributor

That won't work if they're from different realms/contexts, right?

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

Why not use a.constructor === RegExp or possibly a?.constructor === RegExp

I guess we could use a.constructor.name === 'RegExp' or a?.constructor?.name === 'RegExp' then.

A simpler way to test this is a instanceof RegExp. RegExp should be the original value of globalThis.RegExp, like the other builtin object constructors. If these constructors are different in a different realm, using the constructor name solves this for actual instances. Too bad if the user refines a RegExp constructor that does something incompatible.

Same problem with a instanceof Date.

@saghul
Copy link
Contributor Author

saghul commented Mar 10, 2024

I followed what seems to be the most used convention to test for Regex objects.

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

I followed what seems to be the most used convention to test for Regex objects.

OK, classical idiom I suppose. I tend to prefer a simpler idiom that does not involve a function call nor allocate memory.

@bnoordhuis
Copy link
Contributor

Right, but this is in the repl where performance isn't much of a concern.

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

Right, but this is in the repl where performance isn't much of a concern.

I still suggest a?.constructor?.name === 'RegExp' and a?.constructor?.name === 'Date'`

@saghul
Copy link
Contributor Author

saghul commented Mar 10, 2024

Changed it to instanceof like we do for Date. It's a repl, so we don't need to go the extra mile IMHO. If the user wants to monkey around with objects, more power to them.

@saghul saghul merged commit f2a91e8 into master Mar 10, 2024
36 checks passed
@saghul saghul deleted the rm-custom-func branch March 10, 2024 15:55
@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

Changed it to instanceof like we do for Date. It's a repl, so we don't need to go the extra mile IMHO. If the user wants to monkey around with objects, more power to them.

Ben's objection was that objects created in a different realm would not be instances of the same constructors. This is correct, but we do not have support in repl for different realms at the moment.

V8 has a global object Realm with functions to create and handle different realms. This seems to be an extension that is not proposed for tc39. Should we implement it?

@saghul
Copy link
Contributor Author

saghul commented Mar 10, 2024

Any chance ShadowRealm is close to that? https://github.com/tc39/proposal-shadowrealm

@chqrlie
Copy link
Collaborator

chqrlie commented Mar 10, 2024

Any chance ShadowRealm is close to that? https://github.com/tc39/proposal-shadowrealm

It goes in the same direction, but with a different approach: different object/function names and semantics

@saghul
Copy link
Contributor Author

saghul commented Mar 10, 2024

Since the old one never got anywhere I'd say we shouldn't support it.

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