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

BuildUrlMixin.urlPrefix() regression #4105

Closed
oligriffiths opened this issue Jan 20, 2016 · 3 comments
Closed

BuildUrlMixin.urlPrefix() regression #4105

oligriffiths opened this issue Jan 20, 2016 · 3 comments
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@oligriffiths
Copy link

Noticed a regression on this line: b5028e6#diff-391afb21553f5f403d61beaf437459c0R232

On 2.3 this trims the leading / https://github.com/emberjs/data/blob/v2.3.2/addon/-private/adapters/build-url-mixin.js#L234

However the new refactor doesn't.

Thus:

path = /path/to/endpoint
host = /
result = //path/to/endpoint

previously it would return /path/to/endpoint

@bmac
Copy link
Member

bmac commented Jan 21, 2016

Hmm, I'm surprised a host with a value of / worked in 2.3 since I assumed it would get pushed into the array and joined with a slash a the end of the function. https://github.com/emberjs/data/blob/v2.3.2/addon/-private/adapters/build-url-mixin.js#L250

path = '/path/to/endpoint'.slice(1)
host = /
result = ['/', 'path/to/endpoint'].join('/') = //path/to/endpoint

either way this seems like a bug that should be fixed.

@bmac bmac added the Bug label Jan 21, 2016
@oligriffiths
Copy link
Author

A / is stripped off the path, thus making a correct path without the //.

On 20 Jan 2016, at 19:42, Brendan McLoughlin [email protected] wrote:

Hmm, I'm surprised a host with a value of / worked in 2.3 since I assumed it would get pushed into the array and joined with a slash a the end of the function. https://github.com/emberjs/data/blob/v2.3.2/addon/-private/adapters/build-url-mixin.js#L250

path = /path/to/endpoint
host = /
result = ['/', '/path/to/endpoint'].join('/') = //path/to/endpoint
either way this seems like a bug that should be fixed.


Reply to this email directly or view it on GitHub.

HeroicEric added a commit to HeroicEric/data that referenced this issue Jan 21, 2016
…s "/"

Fixes emberjs#4105

I couldn't think of a better name for the test ¯\_(ツ)_/¯
HeroicEric added a commit to HeroicEric/data that referenced this issue Jan 21, 2016
Fixes emberjs#4105

I couldn't think of a better name for the test ¯\_(ツ)_/¯
@oligriffiths
Copy link
Author

awesome! thanks

bmac pushed a commit that referenced this issue Jan 21, 2016
Fixes #4105

I couldn't think of a better name for the test ¯\_(ツ)_/¯

(cherry picked from commit 89f124b)
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

No branches or pull requests

3 participants