fix: prevent double initialization

[jlengstorf]

(NOTE: don’t merge this until #283 goes in)

when setting the URL, the initialize function needs to be called or else the widget gets stuck. however, when no change is required, the init function was being called twice anyways, which caused some undesirable behavior

this adds a check to fix that

[netlify]

Deploy Success!

Built with commit 280d898

https://deploy-preview-284--identity.netlify.app

[erezrokah]

I'm a bit confused by this.
Looking at the code I initially thought we could use the change argument passed to the handler instead of relying on localStorage, but then realised that it doesn't makes sense for mobx to report identical values.

Did a quick test and:

const store = observable({
    siteURL: null,
});
observe(store, "siteURL", (change) => {
    console.log(JSON.stringify(change));
});

store.siteURL = "hello";
store.siteURL = "hello";

Only prints a log message on the first assignment to siteURL, so I'm not sure how we're getting identical values here. The only thing I can think of is that localStorage and store. siteURL become out of sync for some reason.

Also, this changes the current behaviour of calling store.init when siteURL is set to null or undefined, but I haven't been able to figure if there are any implications to that.

[jlengstorf]

@erezrokah as far as I can tell this is only affected by localhost, so I don't think it poses any issues. it's hard to say for sure because I only sort of get how mobx works, so I was just testing workflows

maybe we can publish a beta and put it on a few sites to test before doing a full release?

[tzmanics]

LGTM

[jlengstorf]

I released a beta netlify-identity-widget@1.8.1-beta.0 so we can try this out

to use it, load the JS from https://unpkg.com/netlify-identity-widget@1.8.1-beta.0/build/netlify-identity-widget.js instead of the standard include URL

@erezrokah do you have projects you want to test this against? it works as expected with the repo examples, but those are pretty bare bones

[erezrokah]

Beta is a good idea, but we had the same issue with the CMS beta versions of trying to figure out how to test the beta channel (eventually we stopped using the beta channel).

I finally had some time to look into the original issue with the repo https://github.com/learnwithjason/stripe-jamstack-subscriptions and I think I was able to trace the underlying issue and wrote a test to reproduce it:
https://github.com/netlify/netlify-identity-widget/compare/fix/double-init-local-2?#diff-1dba658c74d9603708d25270ad6e3383R166

The init event is triggered each time the gotrue instance changes:

netlify-identity-widget/src/netlify-identity.js

Line 168 in e1a235b

store.gotrue && trigger("init", store.gotrue.currentUser());

On initial load we attempt to create a new instance here:

netlify-identity-widget/src/netlify-identity.js

Line 249 in e1a235b

store.init(instantiateGotrue(APIUrl));

So far so good.

The thing is that instantiateGotrue sets the siteURL in a specific case (localhost and existing netlifySiteURL in localStorage):

netlify-identity-widget/src/netlify-identity.js

Line 109 in e1a235b

store.setSiteURL(siteURL);

which will end up calling instantiateGotrue again since we are observing changes in siteURL:

netlify-identity-widget/src/netlify-identity.js

Line 150 in e1a235b

observe(store, "siteURL", () => {

In both cases instantiateGotrue will return a new GoTrue instance:

netlify-identity-widget/src/netlify-identity.js

Line 110 in e1a235b

return new GoTrue({ APIUrl: parts.join(""), setCookie: !isLocal });

Which is set to the store here:

netlify-identity-widget/src/state/store.js

Line 42 in e1a235b

store.gotrue = gotrue;

Which triggers the init event.

The solution was to return null on the first instantiateGotrue call and inside the siteURL observer call instantiateGotrue again with the relevant apiUrl.

[jlengstorf]

@erezrokah thanks so much for doing this research! I’m not sure where to go from here, though — do you want me to close this PR and you'll open a different one? or should we make the patch here instead?

[erezrokah]

I’m not sure where to go from here

I should have been more clear about that. I cherry picked the commit from that branch into this PR, did another rebase and pushed the changes.

I'm good with merging if your are.