Mutable List interface for children and childNodes

[fsw]

dart:html allowed modifying child elements and nodes using List interface:

import 'dart:html';

document.querySelector('#foo')!.children.removeWhere((e) => e.classes.contains('remove-me'));

This was a pretty clear and handy interface IMHO.

As discussed in this PR dart-lang/site-www#7040 with @srujzs it might be useful to implement something similar in package:web. This PR is my attempt at doing so.

It adds JSLiveNodeListWrapper that requires a container node and a HTMLCollection or NodeList that is live and implements mutable List methods.

For easier usage it also adds childrenAsList and childNodesAsList via extension so you can do:

import 'package:web/web.dart';

document.querySelector('#foo')!.childrenAsList.removeWhere((e) => e.classList.contains('remove-me'));
document.querySelector('#foo')!.childNodesAsList.removeWhere((e) => e.textContent == 'remove-me');

Questions/Notes:

• Does this implementation make sense and matches ideas behind helpers in this module? JSLiveNodeListWrapper don't have to extend JSImmutableListWrapper but have its own implementation but it kinda felt it makes sense this way.

• Alternatively handy methods like removeWhere could be added via extension directly to HTMLCollection and NodeList but methods like add() might not be possible as those require a parent node to add elements to. This would require knowing if NodeList was returned from querySelector or childNodes and in further case accessing parent somehow. Also implementing full List interface might have other pros.

• could/should NodeList (returned f.e. from querySelector) be extended with asImmutableList via extension? I think it would be more similar to above and dart:html interfaces and JSImmutableListWrapper and JSLiveNodeListWrapper could then be used internally or even marked as private.

• While doing this I noticed ListMixin and ListBase is going to be deprecated in core? Could implementing List interface directly by both those wrappers while making changes here give any benefits?

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

[gemini-code-assist]

Summary of Changes

Hello @fsw, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the package:web library by reintroducing a convenient mutable List interface for interacting with DOM child elements and nodes. By implementing a JSLiveNodeListWrapper and providing extension methods, developers can now perform common DOM manipulation tasks using standard Dart List operations directly on live HTMLCollection and NodeList objects, improving developer experience and code readability.

Highlights

• Mutable List Interface for DOM Nodes: Introduces a mutable List interface for children and childNodes in package:web, similar to the functionality previously available in dart:html.
• New JSLiveNodeListWrapper Class: Adds JSLiveNodeListWrapper to enable mutable operations on live HTMLCollection and NodeList objects, requiring a container node for additions and removals.
• Extension Methods for Convenience: Provides childrenAsList (for Element) and childNodesAsList (for Node) extension methods, allowing direct use of Dart List methods like removeWhere and add.
• Changelog and Tests: Updates the CHANGELOG.md to reflect the new features and adds comprehensive tests in helpers_test.dart to validate the mutable list operations.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a very useful feature, providing a mutable List interface for childNodes and children, similar to the old dart:html API. The overall approach of using a wrapper class is solid.

My review includes a critical fix for a type safety issue in JSLiveNodeListWrapper that could lead to incorrect behavior when manipulating lists of elements. I've also included a few medium-severity suggestions to improve documentation, robustness, and test coverage.

Regarding your questions:

• Extending JSImmutableListWrapper is a good approach to reuse code.
• Your chosen implementation is more powerful than adding extension methods like removeWhere directly, as it provides the full mutable List API.
• Adding an asImmutableList for static NodeLists is a great idea for API consistency and could be a good follow-up.
• ListMixin is not deprecated and is the correct tool for this job.

[fsw]

• ListMixin is not deprecated and is the correct tool for this job.

For clarification, I am talking about this comments:

https://github.com/dart-lang/sdk/blob/9a049d5b2defab33aa953e7882492639ce1f677b/sdk/lib/collection/list.dart#L33
https://github.com/dart-lang/sdk/blob/9a049d5b2defab33aa953e7882492639ce1f677b/sdk/lib/collection/list.dart#L585

[srujzs]

Does this implementation make sense and matches ideas behind helpers in this module? JSLiveNodeListWrapper don't have to extend JSImmutableListWrapper but have its own implementation but it kinda felt it makes sense this way.

I think so! I'll say that it might better if we started from the dart:html implementations instead. I say this mostly because if users are trying to use these helpers to bridge gaps, divergence in behavior would be painful to discover. See _ChildNodeListLazy and _ChildrenElementList. It's fine if we think some of the dart:html implementation methods need to be updated, though.

could/should NodeList (returned f.e. from querySelector) be extended with asImmutableList via extension? I think it would be more similar to above and dart:html interfaces and JSImmutableListWrapper and JSLiveNodeListWrapper could then be used internally or even marked as private.

Can you elaborate on what you mean here? Do you mean only offer an extension that does the wrapping?

While doing this I noticed ListMixin and ListBase is going to be deprecated in core? Could implementing List interface directly by both those wrappers while making changes here give any benefits?

I don't think they are but those comments are indeed suspect. @lrhn, is this something we should worry about?

[fsw]

Does this implementation make sense and matches ideas behind helpers in this module? JSLiveNodeListWrapper don't have to extend JSImmutableListWrapper but have its own implementation but it kinda felt it makes sense this way.

I think so! I'll say that it might better if we started from the dart:html implementations instead. I say this mostly because if users are trying to use these helpers to bridge gaps, divergence in behavior would be painful to discover. See _ChildNodeListLazy and _ChildrenElementList. It's fine if we think some of the dart:html implementation methods need to be updated, though.

Hmm, I was trying to reuse what we have in JSImmutableListWrapper and put both in single implementation.
But having separate implementation for both cases can indeed be beneficial. I will try to rewrite this.

could/should NodeList (returned f.e. from querySelector) be extended with asImmutableList via extension? I think it would be more similar to above and dart:html interfaces and JSImmutableListWrapper and JSLiveNodeListWrapper could then be used internally or even marked as private.

Can you elaborate on what you mean here? Do you mean only offer an extension that does the wrapping?

Precisely. Something like:

extension NodeListExtension on NodeList {
  /// Returns node list as a modifiable [List].
  List<Element> get asList => JSImmutableListWrapper(this);
}

To have a somehow unified interface. (field is native, fieldAsList implements list, querySelector(X) is native and querySelector(X).asList implements list)

[ykmnkmi]

If we have List<JSAny?>.toJSProxyOrRef can we have JSArray.toDartProxyOrRef?

[srujzs]

Hmm, I was trying to reuse what we have in JSImmutableListWrapper and put both in single implementation. But having separate implementation for both cases can indeed be beneficial. I will try to rewrite this.

Yeah, I think they're mildly different enough that splitting it makes sense. Trying to reach parity with dart:html semantics would make migrations easier. I'll re-review whenever you get around to it (no rush).

To have a somehow unified interface. (field is native, fieldAsList implements list, querySelector(X) is native and querySelector(X).asList implements list)

Sure, that's a great goal and will help make these helpers more organized. I'm okay with still exposing the lists for whatever reason a user might want them for (maybe even extending them?) though.

As I'm working on other migrations, I'll clean up the helpers here anyways to make external migrations easier.

If we have List<JSAny?>.toJSProxyOrRef can we have JSArray.toDartProxyOrRef?

That's what toDart more or less already does :D. toJSProxyOrRef is really a performance cliff though, I would be very careful with that.

  /// Converts this [JSArray] to a [List] by either casting or wrapping it.
  ///
  /// > [!NOTE]
  /// > Depending on whether code is compiled to JavaScript or Wasm, this
  /// > conversion will have different semantics.
  ///
  /// When compiling to JavaScript, core [List]s are `Array`s and therefore, if
  /// the [JSArray] was already a <code>[List]<T></code> converted via
  /// [ListToJSArray.toJS], this getter simply casts the `Array`. Otherwise, it
  /// wraps the `Array` with a [List] that casts the elements to [T] to ensure
  /// soundness.
  ///
  /// When compiling to Wasm, the [JSArray] is wrapped with a [List]
  /// implementation and the wrapper is returned.
  ///
  /// Modifications to this [JSArray] will affect the returned [List] and vice
  /// versa.

[fsw]

As suggested by @srujzs, I have rewritten this to be based on current implementation in dart:html instead of extending JSImmutableListWrapper.

I have added a _LiveNodeListMixin though to have a place for shared logic and avoid repetition.

Notable differences from dart:html implementation:

• Support for removeRange was added. Also setRange, replaceRange, setAll, shuffle, sort is now supported via ListMixin (all those were throwing Unsupported exception before)

• insertAll was not supported for HTMLCollection but now it is supported in both cases.

• methods that were almost exactly the same were refactored a bit to be usable in both cases. For example addAll in case of _ChildrenElementList was handling a case of adding elements from another or same live NodeList differently than _ChildNodeListLazy. For example calling children.addAll(children) would throw an exception while calling nodes.addAll(nodes) would simply do nothing. This was unified.

• children.clear() was removing all nodes (including text nodes). I assumed this is not expected / bug and in this implementation this will remove only element nodes.

[srujzs]

Looking good, thanks! Mostly a set of nits.

For example addAll in case of _ChildrenElementList was handling a case of adding elements from another or same live NodeList differently than _ChildNodeListLazy.

That's reasonable. The one thing that's slightly different when conflating the two is if you're doing is checks for _LiveNodeListMixin. For example HTMLCollectionListWrapper.addAll with a HTMLCollectionListWrapper as an argument would pass that is check, but the strictEquals checks ends up making the behavior the same anyways.

children.clear() was removing all nodes (including text nodes). I assumed this is not expected / bug and in this implementation this will remove only element nodes.

Yeah, that seems wrong. I'd just explain the discrepancy as a comment though because it may be a surprise later.

[srujzs]

nit: Do we need a separate iterator class vs toList().iterator like dart:html does instead?

[srujzs]

I'm guessing this is an optimization over having to check the contents of the list.

[srujzs]

This is a no-op in _ChildNodeListLazy. Maybe we should return?

[srujzs]

I think we can just short-circuit if removeMatching is false?

[srujzs]

Same comment as above.