Skip to content
21 changes: 9 additions & 12 deletions test/router.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ describe('Router', () => {
history.replaceState(null, null, '/');
});


it('should throw a clear error if the LocationProvider is missing', () => {
const Home = () => <h1>Home</h1>;

Expand Down Expand Up @@ -611,16 +610,14 @@ describe('Router', () => {
});
}

// TODO: These tests are rather flakey, for some reason the additional handler
// occasionally isn't running prior browser actually navigates away.
// TODO: Need a better way to test these. We're currently racing the browser and trying to observe
// the navigation event after is has been ignored by the router, but before any navigation occurs,
// which has been incredibly flakey. Not to mention, wtr runs in an iframe so `_top` and `_parent`
// wouldn't act as expected either (cheers to @sbesh91 for that).
//
// For now, we can skip, though it'd be good to get a better option in place.
for (const target of shouldNavigate) {
it(`should allow default browser navigation for links with ${getName(target)}`, async () => {
// Currently cross-window navigations, (e.g., `target="_blank"`), do not trigger a
// `navigate` event, which makes this difficult to observe. Per the spec, however, this
// might be a bug in Chrome's implementation:
// https://github.com/WICG/navigation-api?tab=readme-ov-file#restrictions-on-firing-canceling-and-responding
if (target === '_blank' || target === '_BLANK' || target === 'custom') return;

it.skip(`should allow default browser navigation for links with ${getName(target)}`, async () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This totally kicked my ass & I had to throw in the towel, dunno how to test that properly without going to like playwright or something & letting navs actually occur.

Alternatively, it's not necessarily a big surface area for issues, we're just carving out a few extra cases we want to ignore on top of what the browser already handles. Maybe it's not a big deal to miss test cases here? Unlikely to screw up our target matcher.

scratch.querySelector(`#${createId(target)}`).click();
await sleep(1);
expect(triedToNavigate).to.be.true;
Expand Down Expand Up @@ -968,12 +965,12 @@ describe('Router', () => {

expect(loc).to.deep.include({ url: '/foo', path: '/foo', searchParams: {} });

navigation.back();
await navigation.back().finished;
await sleep(10);

expect(loc).to.deep.include({ url: '/', path: '/', searchParams: {} });

navigation.forward();
await navigation.forward().finished;
await sleep(10);

expect(loc).to.deep.include({ url: '/foo', path: '/foo', searchParams: {} });
Expand Down
Loading