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

ignore url query strings in file path #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alancmclean
Copy link

URL Query strings are included in the file path causing "file not found" errors. This should workaround that. Nothing fancy, just a split on the first ? encountered in the file path.

@mbostock
Copy link
Owner

Better would be something like this:

function defaultFile(url) {
  var i = url.indexOf("/", 1) + 1,
      j = url.indexOf("?", i + 1);
  return decodeURIComponent(url.substring(i, j < 0 ? url.length : j));
}

But of course this is still a pretty poor URL parser, but it’s probably okay for something intended to be overridden.

Best would to add some tests. ;)

@alancmclean
Copy link
Author

Yep, this is a lot nicer. Will update.
Introducing tests seems like a separate PR maybe? Got a preference for testing?

@mbostock
Copy link
Owner

That was more a note to myself re. tests. But I’d probably use vows (which is a little long in the tooth but I haven’t bothered to learn a replacement yet) as seen in the smash repo.

@alancmclean
Copy link
Author

Updated with your feedback verbatim.

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

2 participants