Add typings for JSRecord and some unsafe extensions for JSObject

[nex3]

This doesn't include JSObject extensions that require types that
aren't defined yet, like JSSymbolicRecord.

[kevmoo]

We should do better on those JS failures in Wasm: dart-lang/sdk#62218

[srujzs]

Thoughts on making this a generic method instead so that the second tuple member can be a generic (and therefore avoid the need for a cast list)?

[nex3]

I'd worry that this would make it too easy to hide the implicit cast. As a user and an API designer, I'd rather have it be explicit when I'm moving from type-definition-guaranteed types to expression-level-asserted types.

If this is a performance concern for JSRecord.pairs, I'd rather address that by making it directly invoke the JS Object.entries().

[srujzs]

I think the cast would still be explicit within the method, but maybe I don't fully understand

I'd rather have it be explicit when I'm moving from type-definition-guaranteed types to expression-level-asserted types.

Maybe you're saying you'd rather have the user downcast as needed? To be clear, I'm imagining something like:

  List<(String, T)> entries<T extends JSAny?>() {
   return [
     for (var entry in _entries(this).toDart)
       ((entry[0] as JSString).toDart, entry[1] as T),
   ];
}

If one of the tuple members wasn't a T, we'd see a cast failure from the cast.

We could also change the type of _entries and I believe entry[1] will be a cast failure at that point.

[kevmoo]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces JSRecord as a map-like wrapper for JS objects and adds several unsafe extensions for JSObject. The implementation is generally good, but I've found a few areas for improvement, mainly related to performance in JSRecord and some minor issues in documentation and tests. My review includes suggestions to optimize method implementations and fix typos.

[nikeokoronkwo]

Thank you for providing this! It's something I've really wanted officially in Dart JS Interop.

I wanted to ask why we only make use of JSRecord with string keys. According to the ES Spec, JS Records can have a string, number or symbol as a key, the same way a Dart Map can have any Dart type. Packages like zod, reduxjs and some others depend on this functionality, and it would be nice to ensure we do not prevent such packages from having undefined behavior (or just not work well at all) in Dart, or prevent users from being able to use such functionality (I currently use this functionality when working with OpenAPI docs for instance, and iterating through error code indices).

I'm mainly saying this because eventually, I'd want to replace the generated types we make using the web generator to these ones, so it'd be nice if these types could match rather than having to remake the same types.

[nex3]

JS objects can't actually have numeric keys; numbers are coerced to strings. You can test this by running the following in the dev console:

const obj = {};
obj[1] = 1;
obj["1"]

Symbols are another matter. I plan to bring over another type, JSSymbolicRecord, which uses JSAny as its key type in place of String. I think it's valuable to have access to both, because there are many places where only strings are expected or allowed, and it's much simpler to work with these types when you don't have to manually wrap and unwrap JSString every time. Then we can use JSSymbolicRecord for places where the higher generality is allowed.

[nikeokoronkwo]

JS objects can't actually have numeric keys; numbers are coerced to strings.

Agreed, but numeric indexes could also still work, and people/js package authors still rely on this functionality anyways.

The main issue I'm having is that the packages I mentioned use custom types (usually unions, but there are instances of enums as well, which are usually numbers underlying) as indices, which do reduce to string eventually, but at compile time it isn't so clear. Especially with the way dart interop code is generated at the moment, such types, if unions, would show as JSAny, and would therefore not fit in such cases (even if we ignored the type, users may want to index with such values rather than having to coerce directly into a string/number).

Symbols are another matter. I plan to bring over another type, JSSymbolicRecord, which uses JSAny as its key type in place of String. I think it's valuable to have access to both, because there are many places where only strings are expected or allowed, and it's much simpler to work with these types when you don't have to manually wrap and unwrap JSString every time. Then we can use JSSymbolicRecord for places where the higher generality is allowed.

In that case, would it be fine if we have JSRecord as is (maybe with a warning or something else not to use this directly but to use the child types), and then have a JSStringRecord and JSSymbolicRecord inheriting from JSRecord, which the majority of users would use? That way, packages that don't use strings for indexing can still work, and the majority of people can still use.

[nex3]

Agreed, but numeric indexes could also still work, and people/js package authors still rely on this functionality anyways.

In principle, you can pass any value to an object key and it'll work—but it'll also coerce the value to a string. This is the way of a loosely-typed dynamic language. But JS interop generally tries to frame the APIs as strictly as possible, and a JS object can't meaningfully hold any keys that aren't strings or symbols. Since other key types will be coerced anyway, it's easy enough to do that coercion eagerly and pass them to the string API.

It's also worth noting that the JSRecord API simply can't work for other types because they can't be symmetric on input and output. You may be able to store a number or other object in an object's key, but when you go to retrieve or list it you'll always get a string.

In that case, would it be fine if we have JSRecord as is (maybe with a warning or something else not to use this directly but to use the child types), and then have a JSStringRecord and JSSymbolicRecord inheriting from JSRecord, which the majority of users would use? That way, packages that don't use strings for indexing can still work, and the majority of people can still use.

The substantial majority of record-style object uses in JS are specifically string keys (I count about 75% Record<string, ...> versus any other Record<...> declaration in TypeScript, and much of the latter seems to be union types with string representations anyway), and as such it makes sense for that to be the default type named JSRecord than for the two types to have symmetrical names.

[srujzs]

@nikeokoronkwo Could the number/enum use-case be better handled if we had extra members that can set/update/etc. numeric keys? Since the numeric keys are stringified, should APIs like entries still return a Iterable<MapEntry<String, V>>? I agree with Natalie that the symmetry is off, but if we think it may make the numeric case easier if we had some members that operated on inputting numeric keys, that's an easier task. And unlike symbols, numbers get stringified so it seems like a natural fit too.

Re: the polymorphic option, we could have a base class for JSSymbolicRecord that would handle the numeric case too, but I don't think we should have the string version inherit from it as we would be forced to use JSString, which is pretty unideal.

[srujzs]

Okay, I think the implementation as is looks good. When we have a JSSymbolicRecord, let's have it inherit from JSAnyRecord (either in that PR or another) so that the numeric/mixed case is supported implicitly. The alternatives, which are either using JSString in this PR so we inherit from JSAnyRecord or having some additional members to only support inputting numbers, are worse.

I think all that's remaining here are some of my review comments and we're good to go.

[nex3]

I think all that's remaining here are some of my review comments and we're good to go.

Oops, guess I missed these! Done.

[srujzs]

Thanks Natalie!

[srujzs]

I think the cast would still be explicit within the method, but maybe I don't fully understand

I'd rather have it be explicit when I'm moving from type-definition-guaranteed types to expression-level-asserted types.

Maybe you're saying you'd rather have the user downcast as needed? To be clear, I'm imagining something like:

  List<(String, T)> entries<T extends JSAny?>() {
   return [
     for (var entry in _entries(this).toDart)
       ((entry[0] as JSString).toDart, entry[1] as T),
   ];
}

If one of the tuple members wasn't a T, we'd see a cast failure from the cast.

We could also change the type of _entries and I believe entry[1] will be a cast failure at that point.