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

incorrect behaviour of "routing-event"s #484

Closed
nightnei opened this issue Mar 19, 2020 · 12 comments · Fixed by #487
Closed

incorrect behaviour of "routing-event"s #484

nightnei opened this issue Mar 19, 2020 · 12 comments · Fixed by #487

Comments

@nightnei
Copy link

nightnei commented Mar 19, 2020

history.pushstate and history.replacestate have 3 parameters. and the third parameter is URL, which is optional.
https://html.spec.whatwg.org/multipage/history.html#dom-history-pushstate

so even if we use window.history.pushState({}, '') - events single-spa:before-routing-event and single-spa:routing-event will be fired and method isActive of every registered app will be run;

our case: we use https://github.com/mapbox/scroll-restorer in our service https://github.com/namecheap/ilc. and scroll-restorer saves scroll position in the state of history. it's important if we use back/forward buttons in a browser. so every time when a user just scrolls page - it uses pushstate and in result - single-spa think that route is changed.

https://github.com/single-spa/single-spa/blob/108292928d225e1fd7c80245f2d6d6128edd4ee5/src/navigation/navigation-events.js#L124`

@nightnei
Copy link
Author

nightnei commented Mar 19, 2020

@joeldenning look at it please ^^^

@StyleT
Copy link

StyleT commented Mar 19, 2020

Hi! Want to add a bit more clarity here...
It looks like that single-spa shouldn't trigger urlReroute on every call of the window.history.replaceState as it doesn't change URL of the page (trigger reroute only on those calls that change URL). Current behaviour makes single-spa kinda incompatible with libraries that use History API to store some data there.

I also checked PRs in which tracking of the replaceState was added and looks like it was done as an extra measure alongside to the tracking of the pushState method.

At the same time I understand that such a change (modification of the replaceState tracking) is a breaking change. So may be it can be added together with some option that will be turned off by default.

And finally (and this may not be aligned with @nightnei), I'm not sure if we need to alter any logic that is related to pushState method.

@StyleT
Copy link

StyleT commented Mar 19, 2020

Related to the #486 & #485

@StyleT
Copy link

StyleT commented Mar 19, 2020

This is how https://github.com/mapbox/scroll-restorer uses replaceState

https://github.com/mapbox/scroll-restorer/blob/1d187db54cefec24e5618fd8a595392a6de742cf/index.js#L62

So it only extends stateObj in current history entry and I believe this shouldn't trigger single-spa reroute.

@frehner
Copy link
Member

frehner commented Mar 19, 2020

Ah interesting. Did you find this because performance became bad as you scrolled?

In any case, I agree with your assessment - single-spa probably shouldn't be calling that if the url itself hasn't changed.

Like @StyleT mentioned, to avoid a breaking change we could provide it as an option (defaulted off), and then on next major release we could default that option to on (but still provide it as an option?)

@joeldenning
Copy link
Member

joeldenning commented Mar 19, 2020

Some background

Within single-spa, calling pushState/replaceState does two things:

  1. Call all activity functions to see what needs to be mounted / unmounted.
  2. Fire a popstate event manually to all routers on the page.

Regarding 1 - It is possible (although not common) for activity functions to check history.state as part of determining whether a single-spa application should be mounted or not.

Regarding 2 - It is also possible (altough not very common) for other microfrontends on the page to check history.state as part of their re-render

Regarding the popstate event

single-spa intentionally deviates from the browser's default behavior when it comes to popstate events. The objective is to allow for multiple routers to exist at the same time in the same page. A router generally calls pushState() or replaceState() before triggering a re-render of its code. What we need in a single-spa world is for pushState() and replaceState() to trigger a reroute of all the microfrontends, instead of just the one that called pushState/replaceState. The way to do that is to fire a popstate event. However, the browser doesn't fire popstate events when you call pushState. This is where single-spa deviates - it does call popstate when you call pushState/replaceState. This generally triggers a re-render in all microfrontends that have route listeners. Without firing a popstate event, only the microfrontend that called pushState/replaceState will be aware that a re-render is needed.

A question

Does the above behavior cause an error within scroll-restorer? Or is the problem the performance cost of 1 and 2 occurring for every pixel scrolled?

@joeldenning
Copy link
Member

joeldenning commented Mar 19, 2020

If our goal is to improve performance, what do you think of the following options for an API?

singleSpa.start({
  urlRerouteOnly: true
})
singleSpa.setUrlRerouteOnly(true)

^ Is this similar to what you are thinking for an API to control this? I think we'd start it off as opt-in and then make it the default in single-spa@6 (whenever that happens)

@StyleT
Copy link

StyleT commented Mar 19, 2020

@joeldenning agree with everything you said in "Some background" & "Regarding the popstate event" sections.

We see some minor performance issues indeed. And it's generally unexpected a little bit :) I'm fully for the urlRerouteOnly/setUrlRerouteOnly() option.

@joeldenning
Copy link
Member

joeldenning commented Mar 19, 2020

Added this in #487

@joeldenning
Copy link
Member

joeldenning commented Mar 19, 2020

Released in https://github.com/single-spa/single-spa/releases/tag/v5.2.0

@joeldenning
Copy link
Member

joeldenning commented Mar 19, 2020

Documented in single-spa/single-spa.js.org#204

@nightnei
Copy link
Author

nightnei commented Mar 20, 2020

@joeldenning you rock! thank you :)

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 a pull request may close this issue.

4 participants