Forcibly firing popstate event when pushState is called. Resolves #224. #291
Conversation
| // https://github.com/CanopyTax/single-spa/issues/224 and https://github.com/CanopyTax/single-spa-angular/issues/49 | ||
| // We need a popstate event even though the browser doesn't do one by default when you call pushState, so that | ||
| // all the applications can reroute. | ||
| urlReroute(new PopStateEvent('popstate')); |
There was a problem hiding this comment.
I think the two options are two do it this way (which violates the browser spec for pushState) or alter all of the single-spa helper libraries to listen to single-spa's routing event listener and forcibly update when it happens
There was a problem hiding this comment.
That’s a good point, and may warrant a bigger discussion?
I almost lean towards not doing things that are different than the spec, because it opens the door to future bugs when we’re firing that event and it shouldn’t be
There was a problem hiding this comment.
I agree -- lets add this to the next core team meeting agenda?
I think i'll merge this in for now since. I really do think the chance of breaking people's app is pretty small ("what's one more re-render?" famous last words), but am open to reversing this decision and code later if we as a core team decided on a different approach.
There was a problem hiding this comment.
Bad news: I think the alternative implementation doesn't work, although I'm not sure why. Just tried it out with single-spa-angular
|
code looks good to me. I do wonder if this has potential side effects that may affect production apps out there that have workarounds in place for this? or could potentially cause something to render twice? I don't know. Just throwing ideas out there |
|
Yeah I do think that it could have some side effects -- and might cause double render. The only alternative I know of is the one i put above, though, and it causes a double render too. Do you have any ideas about another way? I think canopy has been "double rendering" because hashchange always fires, and it has been fine |
|
Commented above about the options |
Resolves #224 and resolves single-spa/single-spa-angular#49