-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bugfix/fix base #10
base: master
Are you sure you want to change the base?
Bugfix/fix base #10
Conversation
…opies of initial state getting pushed on load
…outing to strictRouting
this.base = this.base.substring(0, this.base.length - 1); | ||
} else { | ||
this.base = base; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this line is redundant :)
this.queryParams = this.getParamsFromUrl(); | ||
|
||
if (base && base[base.length - 1] === '/') { | ||
if (base && base[base.length - 1] === '/' && base !== '/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't the second case take care of the third? if the default of base
is /
then the length would be one, with a minus one would return /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would think so, but it wasn't working for me until I added that. I'll check again and update the pr if it's not needed
} | ||
|
||
//get path we're currently on | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are fantastic
//ignoring this rule for strict routing seems to be the only option right now | ||
//Need to come up with a working solution for "redirects" | ||
//Using hash or pushState for redirects breaks the browser's back button | ||
if (initialLoad === false || redirect === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be if (!initialLoad || redirect) {
This looks great, nice work |
@747823 can you squash your commits and then I'll merge? |
this.wasChangedByUser = true; | ||
window.location.hash = newUrl; | ||
} else { | ||
window.history.pushState(null, null, newUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per MDN second arg should be empty string and maybe first arg should be empty object
* Actual routing function | ||
* | ||
* @param {string} route - New path to naviate to. Absolute path, not including base | ||
* @param {Boolean} isQueryParam - ??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what it does, was waiting for someone else to edit :)
/** | ||
* Actual routing function | ||
* | ||
* @param {string} route - New path to naviate to. Absolute path, not including base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: navigate incorrectly spelled and also the wording is kinda off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dig your use of "nit" @chrisrng.
Had to look it up to confirm it meant what I thought it did and found it in the chromium glossary.
https://www.chromium.org/glossary
short for "nitpick"; refers to a trivial suggestion such as style issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general nit spacing for comments shouldn't be based on whitespace
I fixed the base param to work with a plain '/' as the base, it did not work at all before.
I also removed the history/hash push state on initial load... this should probably be reworked more later, but for me it was causing a bug where I would have an extra duplicate history state in my browser history after first hitting the app using the router. May need to only push the state on initial load if the base isn't already in the window path, or we are using hash and the hash isn't already in the window path. Presumably in most use cases though, the user will be entering at a valid path because the server will 404 other paths, so I didn't change it yet.
Another change here is: I renamed "notStrictRouting" to "strictRouting" for readability. I also made it false by default, because the way we are doing "redirects" is somewhat flawed and it always causes extra duplicate history states to get pushed to the browser history, making the back button not behave exactly as the user might expect. So I think the recommendation should be to leave strictRouting false, and make your SPA render a 404 page (or something like that) if the new route object was empty. I.e. prefer to handle these situations on a per-app basis instead of in the router.
Hope this all makes sense. I am going to merge this in my fork, so I hope ya'll agree with the changes. I know this is a lot of code changes for a single "bugfix" but I feel like these are some fundamental issues that can't really be tackled separately