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

Fire controlling event more than once (#2817) #2857

Merged

Conversation

rockwalrus
Copy link
Contributor

R: @jeffposnick @tropicadri

Fixes #2817

Description of what's changed/fixed.
Dispatches the controlling event every time the documentation specifies that it should.

Dispatches the `controlling` event every time the documentation 
specifies that it should.
@google-cla
Copy link

google-cla bot commented Jun 1, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@rockwalrus
Copy link
Contributor Author

rockwalrus commented Jun 1, 2021

My employer, State Farm, has signed a CLA. Can you tell me who the point of contact is to be added to the list of people authorized to contribute?

@tropicadri
Copy link
Collaborator

@rockwalrus I just want to confirm that you are using your State Farm email to create the commit?

@jeffposnick
Copy link
Contributor

Just as an FYI, we're bundling up a few "ideally not breaking, but let's be careful and try them out first" changes into an upcoming 6.2.0-alpha.0 release. This PR will fall into that bucket, as it fixes what's arguably a bug, we also want to make sure we give folks a chance to test out this updated behavior and ensure it doesn't break their existing assumptions.

We haven't forgotten about the PR, but are wrapping up a few other changes first. Thanks!

@rockwalrus
Copy link
Contributor Author

@rockwalrus I just want to confirm that you are using your State Farm email to create the commit?

Yes, I am using my State Farm email for the commit.

@jeffposnick
Copy link
Contributor

Thanks again for this contribution, @rockwalrus!

There's currently at least one failing test:

1) [workbox-window] Workbox
       register
         reports all events for an external SW registration:

      controllingSpy
      + expected - actual

      -2
      +1

@jeffposnick
Copy link
Contributor

(I'm happy to fix up the tests if you don't mind.)

@rockwalrus
Copy link
Contributor Author

(I'm happy to fix up the tests if you don't mind.)

Go ahead! There seems to be some flakiness in the tests on my machine that I haven't been able to isolate yet. Thanks!

@jeffposnick jeffposnick self-requested a review June 29, 2021 19:35
@jeffposnick
Copy link
Contributor

So it turns out that I don't have permission to push directly to the StateFarmIns branch used in this PR—I thought that GitHub allowed a repo to push to any fork's branch when there's an open PR, but perhaps your repo is locked extra tight?

In any case, here are the three lines that should get the tests passing in the test/workbox-window/integration/test-all.js file: v6...2857#diff-538225be460da9f1c8e38145ed5058866bf568217386107582e0508a7922966b

Hopefully you can apply that change yourself; otherwise, I think we'd have to close this PR and I would open a new one (giving you credit in the release notes, of course).

@google-cla
Copy link

google-cla bot commented Jun 29, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@tropicadri
Copy link
Collaborator

By the way this is the person you need to check with about being a verified contributor for the CLA: Elizabeth Fox Anvick (Infrastructure Analyst-Open Systems) elizabeth.foxanvick.lpjz@statefarm.com

@rockwalrus
Copy link
Contributor Author

So it turns out that I don't have permission to push directly to the StateFarmIns branch used in this PR—I thought that GitHub allowed a repo to push to any fork's branch when there's an open PR, but perhaps your repo is locked extra tight?

I swear that used to be possible, but it's possible something has been changed.

In any case, here are the three lines that should get the tests passing in the test/workbox-window/integration/test-all.js file: v6...2857diff-538225be460da9f1c8e38145ed5058866bf568217386107582e0508a7922966b

Hopefully you can apply that change yourself; otherwise, I think we'd have to close this PR and I would open a new one (giving you credit in the release notes, of course).

Merged and pushed.

@rexb1971
Copy link

By the way this is the person you need to check with about being a verified contributor for the CLA: Elizabeth Fox Anvick (Infrastructure Analyst-Open Systems) elizabeth.foxanvick.lpjz@statefarm.com

Elizabeth has moved to another position in the company and no longer seems to have access to this group. How do we go about recovering access to https://groups.google.com/d/forum/state-farm-insurance?

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

Thanks again for contributing this change! There is some unrelated flakiness in the workbox-broadcast-update tests that I'll look into separate from this PR, so as not to delay things further.

@jeffposnick jeffposnick merged commit 0b5d9d8 into GoogleChrome:v6 Jul 1, 2021
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.

Multiple controlling events
4 participants