Add typings for JSRecord and some unsafe extensions for JSObject #487
Conversation
This doesn't include JSObject extensions that require types that aren't defined yet, like JSSymbolicRecord.
|
We should do better on those JS failures in Wasm: dart-lang/sdk#62218 |
| /// The operation is equivalent to doing `this[key] = value` for each key and | ||
| /// associated value in [other]. It iterates over [other], which must therefore | ||
| /// not change during the iteration. | ||
| void addAllRecord(JSRecord<V> other) => addPairs(other.pairs); |
There was a problem hiding this comment.
tiny nit: maybe addAllFromRecord instead?
| @JS('Object.values') | ||
| external JSArray<JSAny?> _values(JSObject object); | ||
|
|
||
| /// Additional instance methods for the `dart:js_interop` [interop.JSObject] |
| /// See [`Object.entries()`]. | ||
| /// | ||
| /// [`Object.entries()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries | ||
| List<(String, JSAny?)> get entries => [ |
There was a problem hiding this comment.
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)?
| /// The [name] must be a [JSString] or a [JSSymbol]. | ||
| /// | ||
| /// [`Reflect.get()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/get | ||
| R getPropertyWithThis<R extends JSAny?>(JSAny name, JSAny? thisArg) => |
There was a problem hiding this comment.
I feel like this and setPropertyWithThis are sufficiently different in name that we may want a Reflect type instead, especially if we're planning to add more Reflect members. Thoughts?
js_interop/test/record_test.dart
Outdated
| ); | ||
| }); | ||
|
|
||
| test("addPairs()", () { |
There was a problem hiding this comment.
nit: did you mean to name this as clear()?
| expect(record.containsKey("foo"), isTrue); | ||
| expect(record.containsKey("baz"), isFalse); | ||
| }); | ||
|
|
There was a problem hiding this comment.
nit: test for containsValue
| () => | ||
| expect(object.getPropertyWithThis("foo".toJS, object), equals(1.toJS)), | ||
| ); | ||
|
|
There was a problem hiding this comment.
nit: test for getOwnProperty
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
3 participants
This doesn't include JSObject extensions that require types that
aren't defined yet, like JSSymbolicRecord.