3.1.0 --> 3.1.1 changes namespace behavior with leading slashes

[mydea]

After upgrading from 3.1.0 to 3.1.1, our app broke. I guess it is due to this commit:

3a25d71

Basically, our namespace was api/v2, and our rootUrl is /app, and our host is https://our-server.com.
Meaning our app runs on https://our-server.com/app/, and the ajax service is configured like this:

// app/services/ajax.js
import AjaxService from 'ember-ajax/services/ajax';

export default AjaxService.extend({
  namespace: 'api/v2',
  host: 'https://our-server.com'
});

Previously, it would correctly build the urls like this: https://our-server.com/api/v2/xxxx. After the upgrade, it would suddenly be https://our-server.com/app/api/v2/xxx.

We could fix it by changing the namespace to /api/v2, but I guess this kind of breaking change should not happen in a patch-level release.

[artzte]

We aren't using namespaces and were also broken by this patch. Easy enough to fix the inconsistency on our side, but yes, this was a breaking update.

[alexlafroscia]

@artzte can you describe the case that you are in, where this patch broke things for you?

[alexlafroscia]

@mydea I'm sorry about the breaking change here.

Can you step into the code and let me know how app is being added to the URL? Without it in host or namespace I'm unsure how that's ending up in the URL being used.

[artzte]

We had to add leading slashes to paths fetched using ember-ajax. We were not doing this consistently.

[Emrvb]

@alexlafroscia

Can you step into the code and let me know how app is being added to the URL? Without it in host or namespace I'm unsure how that's ending up in the URL being used.

Generally, urls without a leading slash are perceived as relative. Nothing is adding "app". That's just the path to the ember app.

In my opinion the mentioned commit should be reverted, unless the ember-data rest adapter changed its behavior as well. (I'm lagging a bit behind on ember-data versions)

[alexlafroscia]

Sure, I can revert that change and address the clean-up of the URL building in the next breaking change of this repo.

I'm not sure how to version that though -- is going back to the previous behavior also a breaking change?

[jherdman]

I think we need to support both right now, and clearly deprecate one or the other.

[alexlafroscia]

I've thought about this a bit and would these two potential plans:

• I cut 4.0.0 right now, from the current release. It's exactly the same as 3.1.1 (including this breaking change). Anyone that's already fixed the problem in their own codebase can upgrade and we're back to "stability"
  • The release notes will explain the situation and reference this conversation
• I cut 3.1.2 that reverts the change that caused this bug. Anyone that isn't on 3.1.1 yet would get that as their next version, "skipping" 3.1.1 that include the breaking change
• I deprecate 3.1.1 or otherwise mark, through NPM, that people should not install that version. I'll see if I can remove the version altogether, but I know that NPM doesn't really like that. A deprecation warning or something might be enough.

Or

• Write more tests around the URL building to ensure that relative URLs are handled correctly again.
• Release that version as 3.1.2
• Whatever fix people applied to their code to fix the regression in 3.1.1 would still be valid in 3.1.2, so it wouldn't break people's code again. It would just make the upgrade from 3.1.0 or earlier to 3.1.2 painless.

Maybe option 2 is a better way to go? Those affected, which do you prefer?

[alexlafroscia]

I also may be confused about just how this broke on people, too. I think I need to write some more test cases for more combinations to see what the expected and actual behaviors are right now and whether they make sense.

[mydea]

I have already updated our app's code, so I'm not really affected anymore anyhow. I do think it would be nice to use option 2, but not sure if the effort is worth it (no clue how much work this would entail). From my perspective, option 1 would also be OK, if it is easier to implement.

[Emrvb]

@alexlafroscia I explained what broke. It's even in the commit message "fix: don't append leading '/' when building url".

An important question here is, is appending the leading '/' a bug or not. You could argue it is, because it prevents you from using relative urls. However, ember-data's (REST)Adapter appears to be very explicit in appending that leading slash.

I would expect both (ember-data and ember-ajax) to behave the same (considering they're both using the host/namespace properties). Now I don't know who started the host/namespace thingy and who copied it, but changing the behavior should be a collaborated effort.

In my opinion: at this time, this behavior shouldn't be in any version.

I'm not sure how to version that though -- is going back to the previous behavior also a breaking change?

Generally speaking, no. Unless the broken version doesn't get unpublished soon enough and the workaround people are using would break things again upon reverting.
In this case, the workaround doesn't seem to conflict with the original behavior.

[alexlafroscia]

Yeah, after thinking about it more it doesn't seem like there's conflicting behavior. It's just a bug. I'm going to write some tests to cover this and release an update to fix it.

I really appreciate the help people have given and the patience while it's been broken!

[alexlafroscia]

By any chance, could anyone try out ember-ajax@3.1.2 with the old versions of their URLs that do not include the leading /?

@pepke41 and I spent a few hours this evening trying to figure out how to reproduce this bug and have not been able to. We wrote some additional test cases to attempt to mirror the host/namespace combinations that people have mentioned and just can't get a test to produce an unexpected result. You can find the tests we wrote in #399

I tried taking the most recent tests and running them against the addon directory from from v3.1.1 and there are a few test failures that would suggest that in some cases, the host setting on the service was being ignored in v3.1.1.

That behavior seems aligned with the original situation described in this issue -- app was introduced because the host setting was ignored, meaning the rest of the URL was interpreted as being relative, meaning it was added to the base URL which included app.

Since the tests all pass on v3.1.2, that would lead me to believe that the change released in that version fixes this issue, but I would love to either confirm that's true with someone that was bitten by this, or have someone suggest a failing test case that we aren't currently covering.