#3391 - Match trailing slash on root to how root was configured.

[kdickerson]

Fixes #3391 - Allow developers to control whether a trailing-slash should be included or not.

[kdickerson]

Is there anything I can do to help move this forward?

I believe this change maintains backwards compatibility and includes a full set of tests, so it should be a low-risk include.

[jgonggrijp]

@kdickerson Sorry for the long silence. I have recently adopted Backbone maintainership (see #4244) and will give your PR high priority.

[qodesmith]

@jgonggrijp It's been many years since I've used Backbone so I don't think my input is relevant here.

[kdickerson]

That would be fine by me.

I was trying to avoid a new configuration option and make it do what the developer expects based on how the developer passed the root parameter, but I recognize that does change the behavior in the case of developers who have passed a value for root that ends in a trailing slash which is currently being stripped and would, with this change, no longer be stripped.

[jgonggrijp]

Thanks @kdickerson. Would you like to adjust the branch yourself or shall I do it? As far as I'm concerned, this can stay within the same PR.

[kdickerson]

I can do it. I'll see if I can get it done today.

[kdickerson]

Pushed an update after rebasing onto master. Maintains existing behavior unless keepRootSlash is passed as an option to History.start(). keepRootSlash opts into new behavior which always matches the trailing slash to how it was configured by the root option.

[kdickerson]

The keepRootSlash option switches the behavior to always respect whether root ends with a slash or not (perhaps it should be respectRootSlash instead). That is _keepRootSlash indicates we should respect the value passed for root and _wantsRootSlash indicates that the passed value for root ends in a slash. So we have at least 3 states we need to track: legacy mode, new-behavior and root ends in slash, new-behavior and root does not end in slash. I don't see a way to account for all three states without both variables.

The problem, specifically, is that the legacy behavior is not "always remove a trailing slash." It sometimes removes it and sometimes adds it (if root is set to an empty string). So we have to know if we're operating in legacy mode or new mode otherwise we can't do the right thing when root is set to an empty string. We could make the new mode mean "always use a trailing slash" but then there's still no way to get the behavior of "never use a trailing slash."

So it seemed like a better option to make the new mode mean "respect the developer's decision about the trailing slash."

I changed the name of two of the existing tests to improve the naming since several tests had the same name and tested different things. Including one of which tested the exact opposite of the name (name said "No trailing slash on root" and then tests that it has a trailing slash).

I believe the only functionality I changed in an existing test was to add a trailing slash to root to cover the legacy case that trailing slashes are dropped. Unfortunately, the way the diff is being parsed makes that hard to see. I can roll those changes back and instead add another test that covers the legacy behavior that removes trailing slashes. I'll be able to get to it tomorrow.

[jgonggrijp]

The keepRootSlash option switches the behavior to always respect whether root ends with a slash or not (perhaps it should be respectRootSlash instead). That is _keepRootSlash indicates we should respect the value passed for root and _wantsRootSlash indicates that the passed value for root ends in a slash. So we have at least 3 states we need to track: legacy mode, new-behavior and root ends in slash, new-behavior and root does not end in slash. I don't see a way to account for all three states without both variables.

I disagree that the third case should be accounted for. The logic in Backbone.history heavy relies on root starting and ending with a slash. #3391 is a complaint about the slash being present in the root but being stripped when the fragment is empty. You need only one variable to keep track of whether the user wants this or not.

The problem, specifically, is that the legacy behavior is not "always remove a trailing slash." It sometimes removes it and sometimes adds it (if root is set to an empty string).

No, this is a misunderstanding. The slash is added back if the rootPath becomes empty because root is '/' and we strip the final slash. root is never set to an empty string; the start method enforces that it starts and ends with a slash on line 1863. The adding back is to ensure that we retain a leading slash in the degenerate case that this is also the trailing slash.

So we have to know if we're operating in legacy mode or new mode otherwise we can't do the right thing when root is set to an empty string.

What, according to you, is "the right thing" when the root is an empty string?

We could make the new mode mean "always use a trailing slash"

Right, this is what I expected you to implement.

but then there's still no way to get the behavior of "never use a trailing slash."

Why do you want a "never use a trailing slash" mode?

So it seemed like a better option to make the new mode mean "respect the developer's decision about the trailing slash."

If that is the intent, I agree that _respectRootSlash or _freezeRootSlash or something like that would be a more appropriate name for _keepRootSlash. However, I am not convinced that this is a good idea. Saying farewell to the assumption that root begins and ends with a slash seems like opening a big can of worms.

I changed the name of two of the existing tests to improve the naming since several tests had the same name and tested different things. Including one of which tested the exact opposite of the name (name said "No trailing slash on root" and then tests that it has a trailing slash).

This is also a misunderstanding. In those two tests, the user has not set a trailing slash on root (in the first, by not setting a root at all, in the second by setting it to 'root'). Hence "no trailing slash on root". The test verifies that Backbone.history still behaves in a sane way in that case, which includes implicitly adding leading and trailing slashes under the hood.

I believe the only functionality I changed in an existing test was to add a trailing slash to root to cover the legacy case that trailing slashes are dropped. Unfortunately, the way the diff is being parsed makes that hard to see.

I could see that you only added assertions to the end of that test, but that still changes the meaning of the test.

I can roll those changes back

Yes, please do. Please, never change existing tests, unless you can prove (in the strongest sense of that word) that they are wrong.

and instead add another test that covers the legacy behavior that removes trailing slashes.

Maybe, but let us focus on getting the assumptions right first.

[kdickerson]

the start method enforces that it starts and ends with a slash on line 1863.

Ah, I had missed that.

I dislike code which force changes input to something else. In this case it resulted in #2656 due to users expecting the code to respect their configured value (and have no trailing slash). And the response to #2656 to always removing trailing slashes led to #3391 because other users also expected the code to respect their configured values (and have a trailing slash). Too bad the original response to #2656 wasn't to respect the configured value rather than to switch the behavior from slash to no slash.

I realized my current version was colored by my original implementation which attempted to maintain as much backwards compatibility as possible using only the value provided for root. The new implementation doesn't need to care about that and we do only need to track "no trailing slash mode" (which is what the existing implementation does with the leading slash understanding in place) or "trailing slash mode" without caring what is passed since the existing code already ignores what's passed as it relates to leading/trailing slashes.

Knowing that start already ignores the slashing of root should significantly simplify the tests since many of them don't need to exist as they don't actually test anything unique.

I'll get these changes pushed out later today.

[jgonggrijp]

I wouldn't say that start ignores the user's preferences; I would rather frame it as normalizing the root. This is somewhat necessary because Backbone needs to work with the reality of how window.location works. Among other things, this means that the leading slash really needs to be there, even if the user doesn't like it. Also, once you glue two paths together, there's going to have to be a slash in between, so at some point, Backbone would have to add that slash if the user didn't provide it.

That said, I agree that there have been many unfortunate changes in the past (see also #4266). In the case of Backbone.history, I think the end result is still quite sensible, but sometimes, we have to put up with serious stupidity only because removing it would be a breaking change. The best (worst) example of this that I know, is the crazy comparison semantic in Underscore's _.min and _.max (you may need to expand hidden comments and hit enter again in the browser's address bar in order to see that particular comment).

Knowing that start already ignores the slashing of root should significantly simplify the tests since many of them don't need to exist as they don't actually test anything unique.

Do you mean the existing tests or your own tests? In the first case, the tests likely originated in a period when the implementation didn't have all the subtleties that it has today. They should probably still be preserved in order to prevent regressions, but if you tell me which tests you think could be removed, I'll consider it.

I'll get these changes pushed out later today.

Great, thanks!

[kdickerson]

Can help but wonder if I saw the normalization of the slashes when I wrote the original commit since this is basically the same as that. I chose the option name trailingSlash since I felt keepSlash was misleading unless one knows the value for root will be normalized (which I don't see in the documentation, and should probably be added). keepSlash sounds like if there's a slash it will stay, but doesn't, to me, suggest that a slash will always be used.

So I think the semantics of trailingSlash which defaults to false works to indicate there will never be trailing slashes or there will always be trailing slashes.

[kdickerson]

FYI, I didn't add any documentation because CONTRIBUTING.md says:

In your pull request, do not add documentation...

I'll update the documentation.

[jgonggrijp]

Good point! That remark refers to the annotated source code, but I totally agree it makes a different impression. I'll create an issue for it, now that you remind me of it.