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

Provide way to explicitly mark receiver as ready to start handling messages #75

Closed
twschiller opened this issue Aug 18, 2022 · 5 comments · Fixed by #77
Closed

Provide way to explicitly mark receiver as ready to start handling messages #75

twschiller opened this issue Aug 18, 2022 · 5 comments · Fixed by #77
Labels
enhancement New feature or request

Comments

@twschiller
Copy link
Collaborator

twschiller commented Aug 18, 2022

Context

  • Currently the messenger receiver starts listening on import
  • An application may have application-specific setup/initialization to do before it's ready to receive messages
  • Currently the application has to be careful not to import the messenger receiver until the rest of the setup has occurred. This is tricky if there are transitive dependencies to the messenger

Related Discussion

Related Issues

@twschiller twschiller added the enhancement New feature or request label Aug 18, 2022
@twschiller twschiller changed the title Provide way to mark receiver as ready Provide way to explicitly mark receiver as ready Aug 18, 2022
@twschiller twschiller changed the title Provide way to explicitly mark receiver as ready Provide way to explicitly mark receiver as ready to start handling messages Aug 18, 2022
@fregante
Copy link
Collaborator

fregante commented Aug 18, 2022

Currently the application has to be careful not to import the messenger receiver until the rest of the setup has occurred.

That will be fixed for #74, which will make the messenger ignore communication until the first registerMethods call. The messenger will be “ready” when we call registerMethods, which we do once per context.

Having a secondary setMessengerReady() function means we still need to make sure to not add further registerMethods calls after that, so it has a similar risk. Given that we only have one messenger/methods.ts file per context, it’s not a problem at the moment.

@twschiller
Copy link
Collaborator Author

That will be fixed for #74, which will make the messenger ignore communication until the first registerMethods call.

IIRC, registerMethods is called by the initInternalApi. So this is implying that the initInternalApi call won't call registerMethods anymore?

Having a secondary setMessengerReady() function means we still need to make sure to not add further registerMethods calls after that, so it has a similar risk. Given that we only have one messenger/methods.ts file per context, it’s not a problem at the moment.

I'm OK to proceed either way. In both cases, I think the would be a warning to designate mis-use

  • Warn if registerMethods is called multiple times
  • Warn if registerMethods is called after setMessengerReady (if we choose to add that method)

What's an ETA on a fix? I'm planning the PixieBrix 1.7.5 release schedule

@fregante
Copy link
Collaborator

Since you merged pixiebrix/pixiebrix-extension#4061, this is no longer a blocking issue. I will likely finish it tomorrow. Parcel (tests) are giving me grief so I had to take a break.

@twschiller
Copy link
Collaborator Author

twschiller commented Aug 18, 2022

this is no longer a blocking issue

It solves the case where it fails 100% of the time, but doesn't completely solve the problem (one user had actually been seeing the bug, or a related bug, reliably pre-1.7.4).

We've already deployed workarounds for our enterprise customers, so will look to get the real fix into the 1.7.5 release

@fregante
Copy link
Collaborator

But I think this wouldn't fix what you're describing. We already have exactly one registerMethod call per context, which currently happens before any init function except for the content script (and that has been fixed by your recent PR).

Even if the private call happens long before registerMessenger, it still happens in the same tick, there is no chance that a message is received between a webext-messenger import and registerMessenger call currently. That's why this change is only to future-proof it rather than fixing current bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

2 participants