Skip to content

Conversation

@boardend
Copy link
Contributor

Partially reverts #17577 and fixes #17751

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sbc100 This is a regression from #17577 - perhaps we should check the other for-of loops there? I wasn't aware that for-of will throw when the input is e.g. null, but for-in does not.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 3, 2022

Can we add a test?

@kripken
Copy link
Member

kripken commented Sep 6, 2022

A test would be good, but as this is a fix for a regression, we should land this before making a new release. If adding a test would take time, we can maybe add the test later?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 6, 2022

A test would be good, but as this is a fix for a regression, we should land this before making a new release. If adding a test would take time, we can maybe add the test later?

Sure we can land this... it looks like there is the potential for follow to try to clean up all the transferredCanvasNames checks in this file.. and add a test.

@kripken
Copy link
Member

kripken commented Sep 6, 2022

Ok, sounds good @sbc100

@boardend I think merging in latest main should fix those CI errors.

@sbc100 sbc100 enabled auto-merge (squash) September 7, 2022 07:48
@sbc100 sbc100 merged commit 678d7d8 into emscripten-core:main Sep 7, 2022
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 25, 2024
This was first broken back in emscripten-core#17577 and then fixed in emscripten-core#17752.

I then broke it again in emscripten-core#22545 (yay!) (see emscripten-core#22620).

This time I will include a test to ensure this doesn't happen again.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 25, 2024
This was first broken back in emscripten-core#17577 and then fixed in emscripten-core#17752.

I then broke it again in emscripten-core#22545 (yay!) (see emscripten-core#22620).

This time I will include a test to ensure this doesn't happen again.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 25, 2024
This was first broken back in emscripten-core#17577 and then fixed in emscripten-core#17752.

I then broke it again in emscripten-core#22545 (yay!) (see emscripten-core#22620).

This time I will include a test to ensure this doesn't happen again.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 25, 2024
This was first broken back in emscripten-core#17577 and then fixed in emscripten-core#17752.

I then broke it again in emscripten-core#22545 (yay!) (see emscripten-core#22620).

This time I will include a test to ensure this doesn't happen again.
sbc100 added a commit that referenced this pull request Sep 26, 2024
This was first broken back in #17577 and then fixed in #17752.

I then broke it again in #22545 (yay!) (see #22620).

This time I will include a test to ensure this doesn't happen again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

library_pthread.js: transferredCanvasNames is not iterable

3 participants