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

Make isRouting more reliable + other fixes #442

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

jpdutoit
Copy link
Contributor

@jpdutoit jpdutoit commented Jun 8, 2024

I set out to fix #311, but in doing so discovered a few other issues

The isRouting value was not very reliable, and could change many times between true and false

This PR changes isRouting to update once to true when routing starts, and then back to false when routing ends - but only after the source has been updated. The ordering will allow one to use isRouting turning false as a trigger for when it is safe to update your document title. I added tests for these.

While doing this I discovered and fixed the following issues:

Question:
I noticed that a transition was started on creation. Is that initial transition important? I currently have it commented out.

Copy link

changeset-bot bot commented Jun 8, 2024

🦋 Changeset detected

Latest commit: 2f05f37

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@solidjs/router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@Brendonovich Brendonovich left a comment

Choose a reason for hiding this comment

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

Tested this inside Mattrax and it seems to be working

src/routing.ts Outdated Show resolved Hide resolved
src/routing.ts Outdated Show resolved Hide resolved
src/routing.ts Outdated Show resolved Hide resolved
src/routing.ts Outdated Show resolved Hide resolved
@jpdutoit
Copy link
Contributor Author

Pulled in your improvements, and added a few minor ones on top

@@ -187,7 +187,7 @@ export function cache<T extends (...args: any) => any>(fn: T, name: string): Cac
return v;
};
}
}) as CachedFunction<T>;
}) as unknown as CachedFunction<T>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not related, but needed for newer typescript versions

@ryansolid
Copy link
Member

Thanks you both.

@ryansolid ryansolid merged commit 10b7d8c into solidjs:main Jun 12, 2024
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 this pull request may close these issues.

None yet

3 participants