Skip to content

Refactor router, fixes #2505 and #2778 #3027

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kfule
Copy link
Contributor

@kfule kfule commented May 29, 2025

Description

This PR refactors the router code to fix two issues (#2505 and #2778).
This will make the router code a little easier to read and reduce the bundle size (8.97kB(gzipped)).

Motivation and Context

I wanted to close two router-related issues (#2505 and #2778) that had been left unresolved for a long time.
Although these issues do not seem to cause any big problems even if they are not fixed, the fixed bundle size seemed to be smaller than I had expected.

How Has This Been Tested?

npm run test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires a documentation update, and I've opened a pull request to update it already:
  • I have read https://mithril.js.org/contributing.html.

kfule added 2 commits May 29, 2025 20:05
…ixes MithrilJS#2778

Code that sets `fireAsync` to null and uses `hashchange` when `pushState` is not supported was removed in v2.2.0. However, some of that code and comments remained.
…rilJS#2505

This ensures that the reset logic is executed even when there is an internal redirect to the default route in case of failure.
@kfule kfule requested a review from a team as a code owner May 29, 2025 11:42
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

While we're at it, could you just make route.set the actual definition and just have everything call route.set instead of setPath?

That not only will save some more gzip size, it'll also make it so people can hack into route change and transparently wrap it for more advanced use cases like capturing previous paths for more detailed analytics.

@kfule
Copy link
Contributor Author

kfule commented May 29, 2025

@dead-claudia Thank you for your review comment.
As you suggested, I changed it so that route.set is always called.. This definitely reduced the bundle size even further (8.96KB gzipped).

@kfule kfule requested a review from dead-claudia May 29, 2025 22:06
The `state` is represented as a three-valued integer, but due to the `sentinel === currentResolver` condition, a boolean state appears to be sufficient in practice. This improves code readability and further reduces the bundle size.
@kfule
Copy link
Contributor Author

kfule commented Jun 1, 2025

@dead-claudia
I found that the router state could be simplified, so I performed further refactoring. This reduced the bundle size to 8.92 KB gzipped.
This is not related to closing the issues, so I will revert it if you request it.

kfule added 2 commits June 1, 2025 22:05
Since the `state` update process has been removed, `onbeforeupdate` is no longer needed. This further reduces the bundle size.
In the `update()` function, even if the `comp` parameter is undefined, it falls back to "div".
This commit slightly reduces the bundle size and optimizes the component fallback process.
@kfule
Copy link
Contributor Author

kfule commented Jun 3, 2025

I added some simple refactoring and reduced the bundle size.

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.

2 participants