Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow wb.removeEventListener to work without an exception #1963

Merged
merged 2 commits into from
Mar 15, 2019

Conversation

donavon
Copy link
Contributor

@donavon donavon commented Mar 15, 2019

R: @jeffposnick @philipwalton

Fixes #1962

Set does not have a remove method. It should be delete.

@jeffposnick jeffposnick self-requested a review March 15, 2019 17:30
@jeffposnick
Copy link
Contributor

Thanks for the catch, @donavon!

One thing that this points to is that the removeEventListener() code path is currently untested. Would you mind if I pushed an additional commit to this PR that added a test to ensure we will catch any regressions?

@donavon
Copy link
Contributor Author

donavon commented Mar 15, 2019

please do

@jeffposnick
Copy link
Contributor

Merging, unless @philipwalton chimes in with an objection related to the code for the new test...

@philipwalton
Copy link
Member

Thanks @donavon! And yeah, embarrassing that we weren't testing the removeEventListener flow.

@jeffposnick I'm fine with merging this as is, but I may refactor the test later and move it into a file that specifically tests the EventTargetShim module

@philipwalton philipwalton merged commit 493ef61 into GoogleChrome:master Mar 15, 2019
@donavon
Copy link
Contributor Author

donavon commented Mar 15, 2019

No prob. Please let me know when and what version I can expect an npm package.

@philipwalton
Copy link
Member

We have a few bug fixes that might warrant a patch version push. @jeffposnick what do you think? I'm fine with releasing this and the getSW() fix right away unless you have a reason to hold off?

@jeffposnick
Copy link
Contributor

No reason to hold off from my side of things.

@philipwalton
Copy link
Member

Ok, I'll do a 4.1.1 release later this afternoon then.

@donavon donavon deleted the bugfix/set-remove branch March 15, 2019 21:14
@philipwalton
Copy link
Member

4.1.1 has been published 🎉

@donavon
Copy link
Contributor Author

donavon commented Mar 16, 2019

Thx!

I also found this on your docs site. It needs updated as well.

https://developers.google.com/web/tools/workbox/reference-docs/latest/workbox-window_utils_EventTargetShim.mjs

@philipwalton
Copy link
Member

Thanks, that file actually shouldn't even be in the docs since it's private. I've filed an issue to fix that: #1964

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.

3 participants