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

Allow pausing listeners at the history level #1270

Closed
beiifeng opened this issue Jan 16, 2022 · 21 comments
Closed

Allow pausing listeners at the history level #1270

beiifeng opened this issue Jan 16, 2022 · 21 comments
Assignees
Labels
enhancement New feature or request

Comments

@beiifeng
Copy link

What problem does this feature solve?

Some time, we watn remove history listener manaul.

For example, in qiankun micro-front. Main-app and sub-app both vue, and beforeRoute guard with main-app. I passed the history and router of the main-app to the sub-app. The sub-app used the history of the main-app, and created the router itself, which caused the route guard in the main-app to fire twice

Example:
https://github.com/beiifeng/vue-router-next-report
Followe readme , from 首页 to Home, then to 首页,we find BasicLayout:afterEach /Home once in console. But find BasicLayout:afterEach twice after we switch to PppDemo1 . Sys2Demo1 and Sys2Demo2 is used api I want, only print once.

It's my commit: https://github.com/beiifeng/vue-router-next/commit/b4cbbd0b80e3713aca45cb8cfa294b037aaa679c

What does the proposed API look like?

router = createRouter({
  history,
  routes,
});

router.removeHistoryListener();
@beiifeng
Copy link
Author

Cant we expose setupListeners too..

@posva
Copy link
Member

posva commented Jan 17, 2022

The sub app will automatically call removeListeners() when unmounted: https://github.com/vuejs/vue-router-next/blob/master/src/router.ts#L1214

The reproduction is huge, can you remove all unnecessary packages? At the very least everything related to linting, formatting, CSS (you have both less and sass in there), TypeScript, and not having a mono repo would be better as well

We could expose removeHistoryListeners() or rather a destroy() method on the router like its history has but I want to make sure this is necessary

@posva posva added need-feedback Waiting for more information needs reproduction This bug hasn't provided a valid reproduction for the bug to be verified labels Jan 17, 2022
@beiifeng
Copy link
Author

@beiifeng
Copy link
Author

It looks like single-spa's error single-spa/single-spa#911

One way to resolve this error is expose history-listener api to let developers control listener manualy.
I want to know is this necessary too.

@yiliang114
Copy link
Contributor

It is a feature of single-spa, that it will fires PopStateEvent events when it wants to instruct all active applications to re-render. #1423

@posva
Copy link
Member

posva commented Jan 22, 2022

@beiifeng I still need you to remove all the dependencies I mentioned plus anything external or not necessary before I can take a look (e.g. no vue-class-component, qiankun, no babel, etc,) https://github.com/beiifeng/vue-router-next-report/blob/main/packages/main/package.json

@yiliang114
Copy link
Contributor

@posva hi posva, I try to simplify this reproduction, I remove many unnecessary dependencies but except qiankun. Because guard hook has called twice only in subApp of qiankun, the main reason is popState event will be triggered in single-spa ( dependency of qiankun) when router has changed.

You can see this repo, also you can see in codesandbox.

one more thing, I have seen someone said it will behave normally if he use vue-router@3.x in qiankun, and i will try again with 3.x.

@posva
Copy link
Member

posva commented Jan 24, 2022

hmm, still not clear what is the best direction here. So far, I think adding a new active property to the history should be enough. That way, qiankun can paus the history instance when necessary:

const history = createWebHistory()
const router = createRouter({ history })
history.active // true
history.active = false // will not trigger navigations in router

Then create a different history instance on the sub app

@beiifeng
Copy link
Author

hmm, still not clear what is the best direction here. So far, I think adding a new listening property to the history should be enough. That way, qiankun can paus the history instance when necessary:

const history = createWebHistory()
const router = createRouter({ history })
history.active // true
history.active = false // will not trigger navigations in router

Then create a different history instance on the sub app

I mean, history listening popstate event, and router.push call window.history.pushState finally, that's ok.

But, qiankun(actually was single-spa) listen popstate event and dispatch one popstate event.

So, the flow was 1.router.push(vue-router) -> 2.window.history.pushState(vue-router) -> 3.triggerAfterEach(vue-router) -> 4.listen and redispatch PopStateEvent(single-spa) -> 5.listen popstateEvent(vue-router) -> 6.triggerAfterEach(vue-router).

If we can stop 5 manual, its easy solve the problem.

@posva
Copy link
Member

posva commented Jan 24, 2022

Sounds good then. Even though single-spa duplicating popstate events is rather a bug (if it really is duplicating them)

@yiliang114
Copy link
Contributor

Can we check whether the origin (I mean add some private property to the first param of pushState) of popState Event is from vue-router itself? Finally the private property will use to judge vue-router should response in popState listener callbacks , and then the property will be ignored or deleted .

@posva
Copy link
Member

posva commented Jan 24, 2022

I don't think that's a good idea because it would just be a hack to specifically support single-spa which seems to already be doing something hacky

Isn't pausing history listeners enough?

@beiifeng
Copy link
Author

@posva
The removeHistoryListener and setupListeners api not only for single-spa, but also make developers control listener as they want.

But pausing history listeners just can use once as

      // ignore the popstate and reset the pauseState
      if (pauseState && pauseState === from) {
        pauseState = null
        return
      }

By the way, i use

watch(router.currentRoute, (val, oldVal) => {
  if (val.fullPath !== oldVal.fullPath) {
    // something here
  }
});

repalce afterEach guard, now.

@posva
Copy link
Member

posva commented Jan 25, 2022

@beiifeng sorry, I don't understand what you are saying.
Listeners are added at the history level and having routerHistory.listening = true // or false seems to be good without exposing too much of unnecessary low level api

@beiifeng
Copy link
Author

@beiifeng sorry, I don't understand what you are saying. Listeners are added at the history level and having routerHistory.listening = true // or false seems to be good without exposing too much of unnecessary low level api

routerHistory.listening = true // or false yeah, better than me.

@posva
Copy link
Member

posva commented Jan 26, 2022

Great, thanks for confirming, I need to be sure before adding it 😆

@posva posva added enhancement New feature or request and removed needs reproduction This bug hasn't provided a valid reproduction for the bug to be verified need-feedback Waiting for more information labels Jan 26, 2022
@posva posva changed the title Cant we expose removeHistoryListener api with createRouter Allow pausing listeners at the history level Jan 26, 2022
posva added a commit that referenced this issue Apr 20, 2022
@posva
Copy link
Member

posva commented May 2, 2022

The final API should be router.listening = false/true. I thought it would be more flexible to just include directly into the router rather than the history instance

@filoxo
Copy link

filoxo commented May 6, 2022

I wanted to chime in with regards to this

single-spa duplicating popstate events is rather a bug (if it really is duplicating them)

The additional PopStateEvent is not a bug but an intentional design decision made by the framework to best interop with multiple routers that may be on the page. We added urlRerouteOnly option a long time ago to handle most cases. But that doesn't guarantee that it works with every single framework, router, browser target, and userland requirement that comes around despite our best efforts. I think the proposed solution is great and we'd love to document this better for Vue + single-spa users.

@posva
Copy link
Member

posva commented May 6, 2022

Thanks for confirming!

posva added a commit that referenced this issue May 9, 2022
posva added a commit that referenced this issue May 13, 2022
@posva posva self-assigned this May 15, 2022
posva added a commit that referenced this issue May 16, 2022
posva added a commit that referenced this issue May 31, 2022
posva added a commit that referenced this issue Jun 10, 2022
posva added a commit that referenced this issue Jun 29, 2022
@posva posva closed this as completed in 58460bc Jun 30, 2022
@cdxxiaomao
Copy link

qiankun主系统设置 router.listening = false 以后,因为关闭了监听,子系统的路由切换以后,地址栏的地址已经变化,但是主系统的currentRoute并没有发生变化,也就造成了主系统router.push的路由与currentRoute相同,路由不会触发任何动作,地址栏也不会随着更新,子系统的路由也就同样触发不了任何的页面切换。
建议listening=false以后,不要直接returnpush动作保留。

@beiifeng
Copy link
Author

beiifeng commented Jul 8, 2023

该api只是表明 vue-router可以让开发者自己控制路由监听行为,不为任何微前端方案提供解决方法,需开发者根据自身情况进行设置。感谢建议。
I think this api just indicate that developer can controlled router listen action by themselves with vue-router, not the solution for micro-font, you can set it according to your own situation. Thanks for your recording.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Status: Done
Development

No branches or pull requests

5 participants