Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Fix client-side onerror handling no working due to missing crossorigin attribute for CDN scripts #764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wentsul
Copy link

@wentsul wentsul commented Apr 23, 2019

Couple things to note:

  • This needs to be set for __webpack_public_path__ but not sure if it should always be set for "critical chunk" scripts.
  • I this fork deployed for a production and I'm now seeing client-side errors. Previously I was seeing "Script error." line 0.
  • More information on why here: https://blog.sentry.io/2016/05/17/what-is-script-error
  • Would to keep the tests but wasn't sure how to update the tests to only check for critical scripts. Changing el.crossOrigin === 'anonymous' was failing so I'm assuming there are other scripts.

@wentsul wentsul changed the title crossorigin script attribute needs to be set for CDN urls in order to support uncaught error handling Fixes client-side onerror handling no working due to missing crossorigin attribute for CDN scripts Apr 23, 2019
@@ -175,13 +175,6 @@ test('`fusion build` app with dynamic imports integration', async () => {
'one extra script after loading new route'
);

t.ok(
Copy link
Member

@rtsao rtsao Apr 23, 2019

Choose a reason for hiding this comment

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

The reason this crossorigin logic exists is because of:
webpack/webpack#6972 (comment)

So this test is verifying that when not using a CDN, no crossorigin attributes are set. I don't think we want to delete these tests. I think this test should remain unchanged.

Instead, there are tests that do use a CDN, and we should verify that crossorigin attributes are set. As you point out, these assertions would probably currently fail.

Copy link
Author

Choose a reason for hiding this comment

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

added tests for CDN case if we want to go that route.

@@ -114,8 +114,8 @@ const SSRBodyTemplate = createPlugin/*:: <SSRBodyTemplateDepsType,SSRBodyTemplat

for (let url of criticalChunkUrls) {
const crossoriginAttr = url.startsWith(__webpack_public_path__)
Copy link
Member

@rtsao rtsao Apr 23, 2019

Choose a reason for hiding this comment

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

Now I'm thinking that this condition might not be the correct thing to check against, since __webpack_public_path__ could be a CDN. Perhaps it should be url.startsWith("/"), but I suppose that doesn't handle the case where the absolute url is same-origin (although this case might not matter in practice).

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the legacy ssr template used webpackPublicPath.startsWith('http') which is similar to what you mentioned. What about checking forCDN_URL?

const crossoriginAttr = process.env.CDN_URL ? ' crossorigin="anonymous"' : '';

@rtsao rtsao changed the title Fixes client-side onerror handling no working due to missing crossorigin attribute for CDN scripts Fix client-side onerror handling no working due to missing crossorigin attribute for CDN scripts Apr 24, 2019
@rtsao rtsao added the ci label Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants