Skip to content

fix(docs-infra): configure Firebase to strip off the .html extension #25999

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

Closed
wants to merge 2 commits into from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Sep 18, 2018

Firebase used to do it automatically (with cleanUrls: true), but it stopped doing it unless the resulting URL corresponds to an existing file (which is not always the case in angular.io; e.g. the resulting URL might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being correctly redirected (e.g. URLs to the archived v2 site, or .html suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the .html extension and redirect (if no other redirect rule matched).

@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs-infra target: patch This PR is targeted for the next patch release labels Sep 18, 2018
@mary-poppins
Copy link

You can preview 52f3a71 at https://pr25999-52f3a71.ngbuilds.io/.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there are actually no cases where we expect there to be a .html extension on a URL in practice?

Do we care about .htm?
LGTM

@gkalpak
Copy link
Member Author

gkalpak commented Sep 18, 2018

I guess there are actually no cases where we expect there to be a .html extension on a URL in practice?

Not afaict 😁
(Basically, we don't care about .html unless it is a redirect or it is targeting an existing file. The first case is covered, because the new rule comes last in the list of redirect rules and second case is cavered, because Firebase will strip .html from all uploaded files (due to the cleanUrls: true config).)

Do we care about .htm?

Not afaict.

Actually, I inteded to add an e2e test, but forgot it. I'll add it later.
(Or in a follow-up PR if this is merged before I get back 😛)

@gkalpak gkalpak added state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 18, 2018
@gkalpak
Copy link
Member Author

gkalpak commented Sep 18, 2018

Blocked on #25997 (for patch branch).

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).
@gkalpak gkalpak force-pushed the build-aio-redirect-html-ext branch from 52f3a71 to 764717a Compare September 19, 2018 09:08
@mary-poppins
Copy link

You can preview 764717a at https://pr25999-764717a.ngbuilds.io/.

@gkalpak
Copy link
Member Author

gkalpak commented Sep 19, 2018

Rebased on master and added an e2e test. This should be good to go once #25997 is merged.

page.sitemapUrls.forEach((url, i) => {
it(`should not redirect '${url}' (${i + 1}/${page.sitemapUrls.length})`, async () => {
await page.goTo(url);
page.sitemapUrls.forEach((path, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this rename?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity 😇
These are paths and it didn't read correctly to have baseUrl + url --> someUrl.
path is a more accurate name for what this variable holds 😃

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed state: blocked target: patch This PR is targeted for the next patch release labels Sep 19, 2018
@kara kara closed this in df5999a Sep 19, 2018
@gkalpak gkalpak deleted the build-aio-redirect-html-ext branch September 20, 2018 07:30
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 20, 2018
angular#25999)

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).

PR Close angular#25999
gkalpak added a commit to gkalpak/angular that referenced this pull request Oct 1, 2018
angular#25999)

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).

PR Close angular#25999
gkalpak added a commit to gkalpak/angular that referenced this pull request Oct 1, 2018
angular#25999)

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).

PR Close angular#25999
alxhub pushed a commit that referenced this pull request Oct 3, 2018
#25999) (#25997)

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).

PR Close #25999

PR Close #25997
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
angular#25999)

Firebase used to do it automatically (with `cleanUrls: true`), but it
stopped doing it unless the resulting URL corresponds to an existing
file (which is not always the case in angular.io; e.g. the resulting URL
might be matched by a new redirect rule).
This change in Firebase hosting behavior resulted in some URLs not being
correctly redirected (e.g. URLs to the archived v2 site, or `.html`
suffixed URLs from 3rd-party sites).

This commit fixes it, by configuring Firebase hosting to strip off the
`.html` extension and redirect (if no other redirect rule matched).

PR Close angular#25999
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants