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

feat: support options.replace in hash router #523

Open
wants to merge 5 commits into
base: v3
Choose a base branch
from
Open

Conversation

Aliath
Copy link

@Aliath Aliath commented Apr 2, 2025

This update adds support for path replacement in the hash router using options.replace, resolving the issue referenced in #522

Copy link

stackblitz bot commented Apr 2, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Comment on lines 42 to 43
? new HashChangeEvent("hashchange")
: new Event("hashchange");
Copy link
Owner

Choose a reason for hiding this comment

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

I can see that this event has also newURL and oldURL properties, maybe we should add them to the options?

Copy link
Author

Choose a reason for hiding this comment

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

yeah that's a good call 👍

@Aliath Aliath requested a review from molefrog April 3, 2025 19:20
Comment on lines 30 to 31
const oldURL = new URL(window.location.href);
const newURL = new URL(newRelativePath, window.location.origin);
Copy link
Owner

Choose a reason for hiding this comment

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

It seems that oldURL and newURL are actually strings according to the spec and they contain the full URL of the webpage including the new hash and everything else.

image

Copy link
Author

Choose a reason for hiding this comment

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

I wondered why TS hadn't figure it out, but forgot it is a JS file 😬

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 0.17%. Comparing base (e7cb5d6) to head (78c850a).

Files with missing lines Patch % Lines
packages/wouter/src/use-hash-location.js 0.00% 15 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##              v3    #523      +/-   ##
========================================
- Coverage   0.17%   0.17%   -0.01%     
========================================
  Files         10      10              
  Lines        578     580       +2     
  Branches       9       9              
========================================
  Hits           1       1              
- Misses       568     570       +2     
  Partials       9       9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Aliath Aliath requested a review from molefrog April 9, 2025 13:49
Comment on lines 30 to 31
const oldURL = window.location.href;
const newURL = new URL(newRelativePath, window.location.origin).href;
Copy link
Owner

Choose a reason for hiding this comment

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

Just to make it more consistent with the rest of the code

Suggested change
const oldURL = window.location.href;
const newURL = new URL(newRelativePath, window.location.origin).href;
const oldURL = location.href;
const newURL = new URL(newRelativePath, location.origin).href;

@Aliath Aliath requested a review from molefrog April 9, 2025 15:08
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.

3 participants