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

hasBasename bugfix #544

Closed
wants to merge 3 commits into from
Closed

hasBasename bugfix #544

wants to merge 3 commits into from

Conversation

hmate9
Copy link

@hmate9 hmate9 commented Nov 20, 2017

Previously it didn't work when prefix had special regex characters (eg $)

@hmate9 hmate9 changed the title hasBasename updated hasBasename bugfix Nov 20, 2017
@hmate9 hmate9 mentioned this pull request Nov 20, 2017
@mjackson
Copy link
Member

I'm uncomfortable merging this because I think that using a regex in the underlying code is probably too complex for our needs. All we really need to do is make a string comparison that asserts the next char is one of the URL-delimiting characters (i.e. /, ?, #) or the end of the string.

Would you be open to doing a more extensive refactoring here?

@Parakleta
Copy link

I use the following successfully:

export const hasBasename = (path, prefix) =>
    path.startsWith(prefix) && "/?#".indexOf(path.charAt(prefix.length)) !== -1;

This obviously works in the delimited case, but in the end-of-string case it also works because the charAt function returns the empty string if the length is past the end, and indexOf always returns 0 when called with the empty string as an argument.

@hmate9
Copy link
Author

hmate9 commented Nov 20, 2017

@Parakleta I added your change to the PR, just also converting path and prefix to lowercase to make sure the matching is case insensitive.

@hmate9
Copy link
Author

hmate9 commented Nov 21, 2017

@mjackson could you please comment on this PR. Is it ready to be merged?

@@ -5,7 +5,7 @@ export const stripLeadingSlash = path =>
path.charAt(0) === "/" ? path.substr(1) : path

export const hasBasename = (path, prefix) =>
new RegExp("^" + prefix + "(\\/|\\?|#|$)", "i").test(path)
path.toLowerCase().lastIndexOf(prefix.toLowerCase(), 0) === 0 && "/?#".indexOf(path.charAt(prefix.length)) !== -1
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason you're using lastIndexOf instead of indexOf here?

@mjackson
Copy link
Member

Thanks for working on this, @hmate9. Looks great. Just curious about the usage of lastIndexOf instead of indexOf.

@Parakleta
Copy link

lastIndexOf with a fromIndex of 0 is functionally equivalent to startsWith in this case but is available everywhere (lastIndexOf is ES1, startsWith is ES6). Alternatively you could use something like path.substr(0, prefix.length) === prefix which is ES3.

Copy link

@Parakleta Parakleta left a comment

Choose a reason for hiding this comment

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

No, don’t do that, performance is a dog. Leave it as it was.

ETA: to expand, indexOf will check every position even though you’re only interested in the first, wasting time. lastIndexOf only checks the first position because it’s the last it can check and you told it to start there. The substr example I gave previously is less confusing, but is wasteful because it creates a new string object to check against (although maybe if JS has interning and is smart this might be free).

IMO, If ES6 is OK (or polyfilled or transpiled) then use startsWith, otherwise use substr if performance is less important than clarity, otherwise use lastIndexOf, maybe with a comment explaining what it does.

@hmate9
Copy link
Author

hmate9 commented Nov 22, 2017

@Parakleta Are you sure about that? According to the docs:

The indexOf() method returns the index within the calling String object of the first occurrence of the specified value.

It wouldn't make sense to look for more than one match if you have already found one.

@Parakleta
Copy link

Parakleta commented Nov 22, 2017

You’re assuming you find a match...

Put this way, the lastIndexOf is O(min(m,n)) for match or no. The indexOf version is O(m) on a match, and O(m*n) => O(n^2) on no match. Quadratic is bad.

@hmate9
Copy link
Author

hmate9 commented Nov 22, 2017

@Parakleta cool, makes sense. Thank you. What do you think @mjackson ? Should I change it back to lastIndexOf?

@hmate9
Copy link
Author

hmate9 commented Nov 29, 2017

@mjackson could we get an update on this please?

@hmate9 hmate9 changed the title hasBasename bugfix jahasBasename bugfix Jan 22, 2018
@hmate9
Copy link
Author

hmate9 commented Jan 22, 2018

ping @mjackson

This is ready to be merged and is pretty useful fix

@hmate9 hmate9 changed the title jahasBasename bugfix hasBasename bugfix Jan 22, 2018
@avrahams1
Copy link

Is this going to be merged soon?

@bjarkehs
Copy link

Any news on this? I just ran into this issue myself.

@ajhyndman
Copy link

ajhyndman commented Jul 20, 2018

I'm running into this too. I also realized that there are 4 other PRs that have been opened since, attempting to fix the same bug.

#608
#607
#604
#566

It's been six months, so maybe another ping for @mjackson is in order.

@marconi
Copy link

marconi commented Sep 19, 2018

we just ran into this as well LGTM! 👍

@mjackson mjackson mentioned this pull request Mar 27, 2019
25 tasks
@mjackson mjackson closed this in baef934 Mar 27, 2019
@mjackson
Copy link
Member

Thanks for the PR, @hmate9, and sorry for sitting on it for so long. I just merged this manually. It will be released in 4.10 and will be present in the 5.0 release as well.

@Parakleta
Copy link

I think you should be using one of substr, startsWith, or lastIndexOf as discussed in #544 (review) in place of the indexOf(prefix.toLowerCase()).

@lock lock bot locked as resolved and limited conversation to collaborators May 26, 2019
forl pushed a commit to forl/history that referenced this pull request Aug 12, 2019
mjackson added a commit that referenced this pull request Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants