Apply clean-base-url to config.rootURL #6600
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7aa0054
to
48610cd
Compare
Turbo87
reviewed
Dec 22, 2016
48610cd
to
502a523
Compare
|
@Turbo87 I added a comment. It doesn't merely ensure leading/trailing slashes since it runs things through |
Turbo87
approved these changes
Dec 22, 2016
|
@homu r+ |
|
📌 Commit 502a523 has been approved by |
homu
added a commit
that referenced
this pull request
Dec 23, 2016
Apply clean-base-url to config.rootURL So here's the deal: this is an improvement but it is actually a change. This method has not changed: - [In 2.4, before all `baseURL` changes.](https://github.com/ember-cli/ember-cli/blob/v2.4.3/lib/broccoli/ember-app.js#L1656-L1663) - [In `master` after changes.](https://github.com/ember-cli/ember-cli/blob/9e674547c82fa6747895e2b5b7f6a53e73d0226f/lib/broccoli/ember-app.js#L1752-L1759) We would populate the `base` tag with `/` if the provided value was falsey, but the config value would always be the original as specified by the user. [Flipping through the switchover commit you can see that we didn't adjust what appears in the config module in any way.](#5792) I'm in favor of this change since this is what we do internally for all usages. Exposing it to the user in the "canonical" format is probably a better decision. However it _is_ a change and could be considered breaking. I require a +1 from @rwjblue and @Turbo87 to ship. Closes #6599. Discarding this also should close #6599.
|
☀️ Test successful - status |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So here's the deal: this is an improvement but it is actually a change. This method has not changed:
baseURLchanges.masterafter changes.We would populate the
basetag with/if the provided value was falsey, but the config value would always be the original as specified by the user.Flipping through the switchover commit you can see that we didn't adjust what appears in the config module in any way.
I'm in favor of this change since this is what we do internally for all usages. Exposing it to the user in the "canonical" format is probably a better decision. However it is a change and could be considered breaking. I require a +1 from @rwjblue and @Turbo87 to ship.
Closes #6599. Discarding this also should close #6599.