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.

Sign up for GitHub

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

Router: Fix addition and removal of empty classnames #67378

Merged
merged 2 commits into from
Nov 28, 2024
Merged

Router: Fix addition and removal of empty classnames #67378

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4928a5e
Router: Fix addition and removal of empty classnames
tyxla Nov 28, 2024
bb9b71f
Actually prevent transition
tyxla Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/router/src/router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,17 @@ export function useHistory() {

await new Promise< void >( ( resolve ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably remove the ?? '' fallback here

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you added it mostly to satisfy types there. Without it, TS will not be happy because classname could be undefined and classList.add() can't work with undefined. Happy to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the early return will tell typescript that the classname is not empty there anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would have been the case if the calls were synchronous, but this is called in an async function.

Copy link
Member

Choose a reason for hiding this comment

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

Currently TypeScript cannot narrow down the type from string | undefined to undefined because the options.transition usage is inside a nested function (the promise constructor). In such a case it's harder to guarantee in what order code is executed.

But you could solve this by extracting most of the code out of the new Promise callback. The only part that needs to be async is the final transition.finished.finally call. Everything before it is sync.

const classname = options.transition ?? '';
document.documentElement.classList.add( classname );
if ( classname ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's something I'm missing here. When can this happen?

There's a ! options.transition check above that is meant to prevent using a transition entirely no?

Copy link
Member

Choose a reason for hiding this comment

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

There's a ! options.transition check above

But that check doesn't prevent the transition from running! It's:

if ( ! options.transition ) {
  performPush();
}

await new Promise( ... );

There is missing return after performPush(). That's probably the actual bug!

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, nice find @jsnajdr. Updated in bb9b71f

Copy link
Contributor

Choose a reason for hiding this comment

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

I swear in my mind, there was an early return haha :)

document.documentElement.classList.add( classname );
}
// @ts-expect-error
const transition = document.startViewTransition( () =>
performPush()
);
transition.finished.finally( () => {
document.documentElement.classList.remove( classname );
if ( classname ) {
document.documentElement.classList.remove( classname );
}
resolve();
} );
} );
Expand Down
Loading