search

bloomreach / spa-sdk

Apache License 2.0
15 stars 16 forks source link

Error: Ambiguous match found for serviceIdentifier: Symbol(PostMessageService) #10

Closed marcin-jaskulski-wttech closed 1 year ago

marcin-jaskulski-wttech commented 1 year ago

Hi,

I'm experimenting with Bloomreach using developers instance. I have React based SPA that uses you SDKs:

    "@bloomreach/react-sdk": "18.0.1",
    "@bloomreach/spa-sdk": "18.0.1",

I get a lot of these errors:

Error: Ambiguous match found for serviceIdentifier: Symbol(PostMessageService)

It's hard to say how to reproduce this issue. It happens from time to time. There are days when it almost never appears and there are days (like today) when this error doesn't let me work efficiently.

My solutions are one of these (or their combination):

Any idea what can be the reason and how to solve it?

image

marcin-jaskulski-wttech commented 1 year ago

This is what I can see in a browser logs: image

marcin-jaskulski-wttech commented 1 year ago

My hypothesis is that the slower bloomreach responses are, the more often this error appears

beetlerom commented 1 year ago

Hello @marcin-jaskulski-wttech thank you for reporting this issue! @joerideg This sounds like it's related to the async refactoring that we introduced with 18.0.1 . Does this ring a bell?

joerideg commented 1 year ago

Hmm yes it sounds like that PostMessage service is bound twice for some reason. Will need to investigate this.

I do not have an immediate solution for you unfortunately. Refreshing the page should be good enough to fix it, but if your connections are slow at that time for some reason that might take a few tries.

joerideg commented 1 year ago

Created https://issues.onehippo.com/browse/SPASDK-162 for progress tracking

beetlerom commented 1 year ago

@marcin-jaskulski-wttech Do you have a repository where we can reproduce this issue? We do not experience this error in our tests. Thank you!

marcin-jaskulski-wttech commented 1 year ago

Unfortunately, it's not publicly available and I cannot share it. I noticed that the issue happens when the browser developer console is opened. Maybe this will help you reproduce it.

jwgmeligmeyling commented 1 year ago

Can confirm this issue, we experience it as well.

jwgmeligmeyling commented 1 year ago

In our case it seems to be that our componets get mounted twice and effects run twice as well caused by <React.StrictMode> since React 18.0.

@marcin-jaskulski-wttech did you recently upgrade to React 18.0 by any chance? Do you use a non-standard setup where the initialize(configuration) call is done in a useEffect(...) call or the configuration is returned from a useMemo call?

@beetlerom I think it should be easier to reproduce the issue on React v18.

mikroware commented 1 year ago

Since commit 2baf5584b318f93704c41af028118ee20ee9b908 the CmsModule is loaded async on initialization. What I think is causing this issue is specifically these lines: https://github.com/bloomreach/spa-sdk/blob/05e905bc70eaae19b81b0f7a08bbc8d62c9209c6/packages/spa-sdk/src/index.ts#L260-L262

In for example <React.StrictMode> or very quick navigation, the initialize function can run twice very shortly after each other. Which will cause this piece of code to run twice.

Obviously the first run will be fine. The CmsService is not bound yet and thus the CmsModule will be loaded async (which will bind the CmsService).

However, what I believe is causing the problem, is the second run very shortly after. Due to the loadAsync from the previous run, the CmsService might not be loaded yet (isBound() == false) and thus the CmsModule is loaded again. This will cause the services to get bind twice. Amongst others, the PostMessageService, which is the first thing the code will call after the loadAsync and seems to cause the error. Because there are 2 of those service loaded, InversifyJS cannot determine which to pick (which is the Ambiguous match found error mentioned above).

When debugging this in our project, some console logs seem to agree with what I did write above. Also preventing the second loadAsync (with some quick nasty code) seems to resolve the error.

The console logs basically seems to show this order:

1. Running in preview mode [A]
2. Loading CmsModule [A]
3. Running in preview mode [B]
4. Loading CmsModule [B]
5. Done CmsModule [A]
6. Getting PostMessageService [A]
7. Done CmsModule [B]  <----- Now the service are added to the container twice
8. Getting PostMessageService [B]
-- CRASH --

What I think will solve this is re-introducing some kind of await onReady function which can wait on the first loadAsync.

joerideg commented 1 year ago

@mikroware thanks for investigating! That was exactly the problem. I was also able to reproduce it in a unit test by calling initialize twice without waiting for the Promise to be resolved. Since the CmsModule only needs to be initialized once in the lifecycle of the SDK I was able to simply assign the Promise to a variable the first time and await the Promise after and for each subsequent call to the initialize method.

Once we have put my fix through the proper process we will create a release (soon) with the fix and then I will notify you here and close this issue. Thanks for collaborating!

joerideg commented 1 year ago

Hi, we just released 21.0.0. This reverts the async behavior and inversify dynamic loading we introduced in 18. This should fix your issue 👍🏻