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

Breaking: Update hint to check for XCTO header on all resources #1842

Merged
merged 4 commits into from
Feb 7, 2019

Conversation

utsavized
Copy link
Contributor

This reverts changes added to check for this header only
on scripts and stylesheets, and instead, checks for the header
on all resources. MDN suggests the former, but Chromium uses
this response header on more than script/stylesheets for
CORB.


Ref #1221

Close #1221

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

This reverts changes added to check for this header only
on scripts and stylesheets, and instead, checks for the header
on all resources. MDN suggests the former, but Chromium uses
this response header on more than script/stylesheets for
CORB.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref webhintio#1221

Close webhintio#1221
@utsavized utsavized added this to the 1902-1 milestone Feb 6, 2019
@utsavized utsavized self-assigned this Feb 6, 2019
@utsavized utsavized requested review from molant and antross February 6, 2019 05:56
@utsavized utsavized requested a review from sarvaje as a code owner February 6, 2019 05:56
Copy link
Member

@molant molant left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Couple comments in the commit comment for future:

  • This should be Breaking instead of new because before we were failing if the header was sent to all resources
  • We should update our examples on how to pass this rule with apache and IIS and update our website's web.config
  • The links at the end should be mostly Fix #ID. The Ref is used to links for further info and Close to indicate the PR that closes the issue. We should probably update the documentation so it's less confusing

I'm going to request changes but just so we don't forget about updating the documentation for the web servers but otherwise LGTM. If you have time to do it it will be awesome. If not I'll try to do it myself later today.

Thanks!

@utsavized utsavized changed the title New: Update hint to check for XCTO header on all resources Breaking: Update hint to check for XCTO header on all resources Feb 6, 2019
@utsavized
Copy link
Contributor Author

@molant Added documentation of server config. Thanks!

Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

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

Looks good.

I'd love to see us report some of this in the future by examining server configuration files directly when available (filed as #1854).

@molant molant merged commit 4f69029 into webhintio:master Feb 7, 2019
@antross
Copy link
Member

antross commented Feb 7, 2019

@molant given the README updates, this touched packages/hint so we're going to get a major version rev the next time we release due to the Breaking tag... Not sure if we already had other breaking changes queued up so this doesn't matter, or if you want to split the commit and re-write history, or just leave it.

@molant
Copy link
Member

molant commented Feb 7, 2019

I missed that 😥
I'll rewrite the history.

molant pushed a commit that referenced this pull request Feb 7, 2019
This reverts changes added to check for this header only on
scripts and stylesheets, and instead, checks for the header
on all resources. MDN suggests the former, but Chromium uses
this response header on more than script/stylesheets for
CORB.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Fix #1221
Close #1842
@molant
Copy link
Member

molant commented Feb 7, 2019

OK, should be fixed now. Nice catch @antross

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants