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

Handle trailing slashes in basename #108

Merged
merged 1 commit into from
Oct 28, 2015
Merged

Conversation

davis
Copy link

@davis davis commented Oct 23, 2015

currently:

// this works
history = useBasename(createHistory)({
  basename: '/base/url'
})
// but this doesn't
history = useBasename(createHistory)({
  basename: '/base/url/'
})

this pr handles both cases

@taion
Copy link
Contributor

taion commented Oct 23, 2015

Do we really need this in the code though? I feel like it'd be easier just to tell people to not put a trailing slash in basename.

@davis
Copy link
Author

davis commented Oct 23, 2015

Intuitively I feel like if the basenames: '/' and '' have the same behavior than so should /base/ and /base

@mjackson
Copy link
Member

I'd be ok with this change. There are failing tests that need to be fixed, though. Can you please fix them first, @davis?

@taion
Copy link
Contributor

taion commented Oct 25, 2015

Test failure is not related to the PR I don't think - I'm re-running the build.

@taion
Copy link
Contributor

taion commented Oct 25, 2015

Oh, never mind, I didn't scroll down far enough - those are real test failures.

@davis
Copy link
Author

davis commented Oct 26, 2015

Hey guys, I've changed the code to use slice/charAt instead of startsWith/endsWith so that the tests pass in all environments.

@taion
Copy link
Contributor

taion commented Oct 26, 2015

Thanks! Do we need to update any docs or changelogs to go with this?

@mjackson
Copy link
Member

We can merge this for now, @taion. I'll add a "contributing" doc that explains how to update CHANGES.md when making PRs.

mjackson added a commit that referenced this pull request Oct 28, 2015
Handle trailing slashes in basename
@mjackson mjackson merged commit 5b0266d into remix-run:master Oct 28, 2015
@mjackson
Copy link
Member

Thanks @davis !

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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.

3 participants