Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add methods to detect arrays #282

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Add methods to detect arrays #282

merged 1 commit into from
Feb 28, 2024

Conversation

rockwotj
Copy link
Contributor

I have a use case where a user can hand me many different kinds of
types, array buffer, uint8array, or a string, and I need to be able to
distingush between them.

I have a use case where a user can hand me many different kinds of
types, array buffer, uint8array, or a string, and I need to be able to
distingush between them.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@chqrlie
Copy link
Collaborator

chqrlie commented Feb 23, 2024

Instead of adding numerous APIs like this, we should consider exporting the JS_CLASS_XXX definitions for the standard types.

@rockwotj
Copy link
Contributor Author

I am happy to do that instead.

What is the best way folks see to do that? Add a bunch of #define constants to the header for builtin classes, add a new enum in the header, or actually move the existing enum into the header

@saghul
Copy link
Contributor

saghul commented Feb 23, 2024

Instead of adding numerous APIs like this, we should consider exporting the JS_CLASS_XXX definitions for the standard types.

I was thinking the same thing!

@saghul
Copy link
Contributor

saghul commented Feb 23, 2024

I am happy to do that instead.

What is the best way folks see to do that? Add a bunch of #define constants to the header for builtin classes, add a new enum in the header, or actually move the existing enum into the header

IMHO moving the enum to the header would be best.

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 23, 2024

I am not sure about moving the whole enum to keep some flexibility in the implementation for internal classes. We should move the standard class ids to the beginning of the enum and publish that enum, then use a separate private enum starting at the end of the public one for internal classes.

@saghul
Copy link
Contributor

saghul commented Feb 23, 2024

Makes sense!

@bnoordhuis
Copy link
Contributor

bnoordhuis commented Feb 23, 2024

As a counterargument: both V8 and JavaScriptCore have a plethora of "is foo" functions. It gives them the flexibility to change object representation at a whim without breaking downstream users.

Exporting an enum means you can't ever change it (only add to it) without breaking source or binary compatibility.

@saghul
Copy link
Contributor

saghul commented Feb 23, 2024

Good point! It also aligns well with isundefined and so on...

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 25, 2024

The "is foo" functions could be defined in quickjs.h as simple inline functions comparing the return value of JS_GetClassID() with a constant for the standard objects, unlikely to change implementation anytime soon, yet it is difficult to know this in advance with certainty. For example, JS_IsString() may change to include both JS_TAG_STRING and JS_TAG_ROPE objects...

So source code compatibility should be sought and more likely achieved than binary API compatibility.

@bnoordhuis
Copy link
Contributor

So source code compatibility should be sought and more likely achieved than binary API compatibility.

I agree source code compatibility will be easier but binary compatibility is a conversation worth having if we want distros to start packaging libquickjs. They're virulently allergic to libraries without a stable API/ABI.

That's more of a roadmap thing and doesn't need to hold up this pull request though. I believe we're in agreement that JS_IsArray() et al are a good way forward, regardless of exact implementation details?

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 25, 2024

Actually, it is a little more complicated: in QuickJS, JS_IsArray() handles the case of proxies and may throw an exception if the proxy has been revoked or if there is a cycle in the proxy chain. Is this the behavior of the equivalent call on Node.js ?

Should other "is foo" API functions function this way ?

For the sake of the principle of least surprise, equivalent calls on different engines should behave in a consistent way.

PS: I have never looked at the Node.js source code :)

@bnoordhuis
Copy link
Contributor

Yes, node.js / V8 behaves the same way w.r.t. proxies. They're transparent unless you specifically ask "is this a proxy?"

@rockwotj
Copy link
Contributor Author

https://github.com/v8/v8/blob/main/include/v8-value.h

It looks like v8 has a bunch of these checks

@rockwotj
Copy link
Contributor Author

Is there anything I can do to get this merged? I am currently hardcoding the constants in my app for these classes.

@saghul
Copy link
Contributor

saghul commented Feb 28, 2024

After some back and forth looks like this is the way we want to go, right folks?

@bnoordhuis bnoordhuis merged commit ec4f957 into quickjs-ng:master Feb 28, 2024
35 checks passed
@chqrlie
Copy link
Collaborator

chqrlie commented Mar 1, 2024

It seems inconsistent that JS_IsArray handles proxies and not JS_IsArrayBuffer or JS_IsInt8Array.
Furthermore, the current behavior of JS_IsArray throwing exceptions for proxy cycles and other proxy failures may not be desirable.

Should we have a flag to specify whether to follow proxy chains and still return a boolean for native objects and safe proxies with just a limited chain depth?

@saghul
Copy link
Contributor

saghul commented Mar 1, 2024

I guess one difference here is that when JS_IsArrayBuffer returns true, JS_GetArrayBuffer should just work, right? AFAIS it will fail because it doesn't handle proxies.

So maybe it should be the other way around?

@rockwotj rockwotj deleted the ab branch April 12, 2024 20:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants