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

Use new demo page #306

Merged
merged 6 commits into from
Mar 5, 2023
Merged

Use new demo page #306

merged 6 commits into from
Mar 5, 2023

Conversation

ota-meshi
Copy link
Member

Which issue, if any, is this issue related to?

None.
See stylelint/stylelint-demo#351

Is there anything in the PR that needs further explanation?

Still WIP. This uses the page URL which is still in development.

@ota-meshi
Copy link
Member Author

Hmm.. Some images won't load because of the added headers. I don't know this solution yet.

image

// but some still fail.
// So we have to reload the page to reapply the header.
// Another reason is that SPA cannot control the header for each page.
window.location.reload();
Copy link
Member Author

Choose a reason for hiding this comment

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

Check the page header state and force a reload and refresh the header if there is a problem with the image url. It's not the best way, but it seems to solve the problem of cross-origin image loading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps if we could separate the demo page from the SPA, we wouldn't need this workaround.
Do you know how to do that?

Copy link
Member

Choose a reason for hiding this comment

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

This issue facebook/docusaurus#3309 may be related, but I cannot find a solution... 😓

Copy link
Member Author

Choose a reason for hiding this comment

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

As another solution, proxying each external resource via netlify might solve the CORS issue, but that might complicate the configuration.

https://docs.netlify.com/routing/redirects/rewrites-proxies/#proxy-to-another-service

@ota-meshi
Copy link
Member Author

Please let me know your thoughts on how to solve the problems related to cross-origin.

Problem details:
Cross-origin isolation is required for demo to work. But this seems to make cross-origin resource access strict. Some external images don't fetch well (yet).

https://developer.mozilla.org/en-US/docs/Web/API/crossOriginIsolated

For example, trying to get backers.svg will result in the following error:

Access to image at 'https://opencollective.com/stylelint/backers.svg?width=840&avatarHeight=48&button=false' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access- Control-Allow-Origin' header is present on the requested resource.

We get the same error with the API below.

https://github.com/stylelint/stylelint/workflows/Testing/badge.svg
https://www.netlify.com/img/global/badges/netlify-color-accent.svg
https://opencollective.com/stylelint/sponsors.svg
https://opencollective.com/stylelint/backers.svg

  • Solution I tried that didn't work:
    We considered how to switch between cross-origin isolation and non-cross-origin isolation on the demo page and other pages (such as the top page).
    Initial load works fine by toggling the header of each page in Netlify config.
    However, since the site is the SPA, an error occurs on the transition destination page when transitioning from the demo page to the top page and from the top page to the demo page. This is because the expected cross-origin isolation state inherits the state of the first opened page.

  • Solution 1:
    Ask the publisher of the API that provides each resource to change the API so that it can be loaded even in cross-origin isolation. That would be ideal, but I'm not sure where to ask for them all.

  • Solution 2:
    Reload each page if it's not in the expected cross-origin isolation state. (The current PR does that.)
    This allows you to maintain the desired cross-origin isolation state for each page. However, it can be said that it is a bit hacky because reload runs after page transition by SPA.

  • Solution 3:
    Separate SPA for demo page and other pages. However, it is expected that it will be difficult due to the mechanism of docusaurus.
    Unability to link to an internal non-SPA link facebook/docusaurus#3309

  • Solution 4:
    We will proxy all of the external resources we use with Netlify and modify external resource access to not use cross-origin. But I haven't tried it yet so I don't know if it will work. I also think it complicates Netlify configuration and conversion scripts. Therefore, I am worried about whether maintenance can be continued well in the future.
    https://docs.netlify.com/routing/redirects/rewrites-proxies/#proxy-to-another-service

@ota-meshi
Copy link
Member Author

ota-meshi commented Feb 28, 2023

Solution 5:
Give up on using WebContainers API 😓

@ybiquitous
Copy link
Member

@ota-meshi Thanks a lot for summarizing!

I think "Solution 2" is the easiest way for now. I don't feel UX is degraded on the preview.

@jeddy3
Copy link
Member

jeddy3 commented Mar 1, 2023

I think "Solution 2" is the easiest way for now. I don't feel UX is degraded on the preview.

I agree. Preview LGTM.

@ota-meshi
Copy link
Member Author

Thank you for your opinion!
I think this PR is almost ready.

All we have to do is change the <iframe> url that this PR refers to after merging stylelint/stylelint-demo#352.

return;
}

const FRAME_ORIGIN = 'https://deploy-preview-352--chimerical-trifle-8d3c21.netlify.app';
Copy link
Member Author

Choose a reason for hiding this comment

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

We probably need to change here:

Suggested change
const FRAME_ORIGIN = 'https://deploy-preview-352--chimerical-trifle-8d3c21.netlify.app';
const FRAME_ORIGIN = 'https://chimerical-trifle-8d3c21.netlify.app';

@ota-meshi ota-meshi marked this pull request as ready for review March 1, 2023 21:50
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Awesome work! 👏🏼

@jeddy3 jeddy3 merged commit d97e5f2 into main Mar 5, 2023
@jeddy3 jeddy3 deleted the new-demo branch March 5, 2023 08:19
@jeddy3
Copy link
Member

jeddy3 commented Mar 5, 2023

🎉 @ota-meshi Thanks again for all your fantastic work on the demo!

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

Successfully merging this pull request may close these issues.

3 participants