search

richnologies / ngx-stripe

Angular 6+ wrapper for StripeJS
MIT License
217 stars 77 forks source link

fix(api-loader): wait until stable to not block hydration #229

Open yharaskrik opened 10 months ago

yharaskrik commented 10 months ago

What are you adding/fixing?

With Hydration in Angular it can only run once the app has become stable, this is not usually a problem but the act of adding the Stripe script to the head delays the app being stable by ~5s (from locally serving SSR), the script itself loads but then it takes some number of seconds to load the trusted types.

This PR updates the lazy api loader service so that it will wait until the app is stable to load the script. It will wait 10 seconds to become stable (same as hydration) and then load the Stripe script anyway.

Will this need documentation changes?

No

Does this introduce a breaking change?

It should not, would depend on if people are expecting the script to be there within a certain timeframe, but if they are they would have race conditions regardless. This change could also be put behind a configuration flag (waitForStable or something) as realistically this change is most likely only needed for those using SSR and hydration.

Other information

richnologies commented 10 months ago

Hey @yharaskrik, very interesting contribution. Just have a few questions:

  1. It seems you no longer call the load method at all, I'm I missing something?
  2. The asStream() method is also called in an imperative way. I'm ok waiting on the site bootstrap, but not if I'm making a directly call and is already loaded
  3. Do you have any documentation regarding this issue?
yharaskrik commented 10 months ago

Hey @yharaskrik, very interesting contribution. Just have a few questions:

  1. It seems you no longer call the load method at all, I'm I missing something?
  2. The asStream() method is also called in an imperative way. I'm ok waiting on the site bootstrap, but not if I'm making a directly call and is already loaded
  3. Do you have any documentation regarding this issue?

Ha! this is why we have code reviews! I forgot to put the load back in, we want the load to be called after the app is stable (or after the timeout) so I can put that back in.

The way I have it setup (if I am understanding correctly, if I am wrong please let me know!), is that when asStream() is subscribed to then that is when we wait for the app to become stable, once it is stable (or stability has timed out) then we would return the status (as a stream) so the result of the asStream method is an Observable of the status.

No documentation as it currently stands beyond a conversation I had with one of the Angular team members who worked on hydration. (Jessica Janiuk) in the GDE slack, I only ran across it when I was figuring out why hydration was not working for us, loading the Stripe script was fine but as soon as it was loaded then a trusted types call was made which prevented the app from becoming stable and was a large part of why hydration times out. There isn't a TON of documentation on hydration right now unfortunately in terms of technical details/issues etc.

I am of course open to other ideas here but what the goal of this was was wait to insert the stripe script until after the app has become stable.

yharaskrik commented 10 months ago

Now that I look at it though you are right in that this isn't going to work how we want. Let me push up a small change.

yharaskrik commented 10 months ago

OK updated it a bit, now anytime asStream() is resubscribed to it will reuse the same isStable value (whether it was stable or had timed out) and then calls load. This ensures that if asStream is called numerous times we don't need to wait the 10s to timeout before load is called again (in the case of an app never becoming stable)

richnologies commented 10 months ago

Very nice! Let me make some tests over the weekend and we can merge and release next week 👍

Thanks again

richnologies commented 10 months ago

Hey @yharaskrik, I've been doing some tests and found a few issues:

  1. If we enter the catchError operator, returning EMPTY means we never load Stripe
  2. If I run the docs and dev mode, I'm only getting on value from isStable and is always false. Is it maybe because we are not using SSR or Hydration for our docs right now?
yharaskrik commented 10 months ago

Hey @yharaskrik, I've been doing some tests and found a few issues:

  1. If we enter the catchError operator, returning EMPTY means we never load Stripe
  2. If I run the docs and dev mode, I'm only getting on value from isStable and is always false. Is it maybe because we are not using SSR or Hydration for our docs right now?

Ah that is good to know, honestly we can return whatever we want from the catchError, the only reason it is there is to catch the timeout so that if the app never becomes stable the script will still end up loading.

The docs might never become stable (that is why the timeout is there, to ensure that in that case the script is still loaded). Stability can be affected by numerous things, one of which is setTimeouts/setIntervals and things like that.

yharaskrik commented 9 months ago

Sorry I haven't had time to come back and make these changes, I fully intend to!

js-padavan commented 6 months ago

Hi guys, I also came across with hydration issue, when application not becoming stable in 10 seconds due to @stripejs internal code scheduling some macro tasks. Is there any chances this PR will be merged soon ?

Also, there could be an alternative solution, loading @stripejs outside of angular zone. Such approach should be more effective, since it will not run unnecessary change detection cycles, and will not wait to load stripe until app become initially stable.

yharaskrik commented 6 months ago

Hi guys, I also came across with hydration issue, when application not becoming stable in 10 seconds due to @stripejs internal code scheduling some macro tasks. Is there any chances this PR will be merged soon ?

Also, there could be an alternative solution, loading @stripejs outside of angular zone. Such approach should be more effective, since it will not run unnecessary change detection cycles, and will not wait to load stripe until app become initially stable.

After some investigation I am not actually sure my solution is going to fix the issue, but loading outside of zone might. If we add the script using JS inside of the run outside of zone will any tasks scheduled by that script that is added also be run outside of zone?