Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

Make Element.attributes iterable #47

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Conversation

jdanyow
Copy link
Contributor

@jdanyow jdanyow commented Jun 13, 2020

enables:

for (const attribute of el.attributes) {

enables:
```ts
for (const attribute of el.attributes) {
```
@jdanyow
Copy link
Contributor Author

jdanyow commented Jul 1, 2020

@harrishancock will this ship in the next release?

@jdanyow
Copy link
Contributor Author

jdanyow commented Jul 1, 2020

My bad, I don't think the change in my PR is quite right.

Current change:

-  readonly attributes: Iterator<{ name: string; value: string }>;
+  readonly attributes: IterableIterator<{ name: string; value: string }>;

The worker runtime actually exposes this interface:

readonly attributes: IterableIterator<[string, string]>;

This means the worker's Element.attributes is not symmetric with the DOM's. Before I update the PR, can you confirm this is intentional?

With the DOM API you'd write:

for (const { name, value } of el.attributes) {
  console.log(name, value);
}

With the HTMLRewriter it's:

for (const [name, value] of el.attributes) {
  console.log(name, value);
}

@jdanyow jdanyow marked this pull request as draft July 6, 2020 19:46
@jdanyow jdanyow marked this pull request as ready for review July 6, 2020 20:04
@harrishancock
Copy link

I'm not sure if the asymmetry with the DOM's API is intentional or not. (/cc @inikulin) It's a little unfortunate, but workers-types should just match what the runtime does and is documented to do, since changing the behavior, if we choose to do so, would take some time.

As for when this can be released, I defer to @ispivey!

@jdanyow
Copy link
Contributor Author

jdanyow commented Jul 6, 2020

@harrishancock @inikulin sounds good- PR matches the docs and workers runtime now.

There's another small divergence from the DOM API with respect to Element.tagName. In the DOM it's always uppercase and in workers it's lowercase. May be worth calling these out in the docs.

@inikulin
Copy link

inikulin commented Jul 6, 2020

Back end returns an object that exposes two getters mimicking DOM API. So, the change will be required in Workers runtime only.

@jdanyow
Copy link
Contributor Author

jdanyow commented Jul 23, 2020

@harrishancock @ispivey I think this is ok to merge. The PR matches the Cloudflare workers runtime as it is today, fixing a bug in the type definitions that prevents writing valid code like this:

rewriter.on('bla', {
  element: element => {
    for (const [name, value] of element.attributes) {  <-----

@ispivey ispivey merged commit 519aebe into cloudflare:master Jul 24, 2020
@koeninger koeninger modified the milestones: Release 2.0.1, Release 2.1.0 Oct 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants