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

Add crossOrigin via props to _document Head and NextScript #5646

Merged
merged 7 commits into from
Nov 13, 2018
Merged

Add crossOrigin via props to _document Head and NextScript #5646

merged 7 commits into from
Nov 13, 2018

Conversation

Enalmada
Copy link
Contributor

@Enalmada Enalmada commented Nov 9, 2018

This alternative implementation of #5150 follows @timneutkens suggestion of using props.

Fixes #5150
Fixes #3630

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

The PR looks great! The only thing missing is a test for this new behavior.

@timneutkens
Copy link
Member

You can add this test to test/integration/app-document by checking the HTML response

@Enalmada
Copy link
Contributor Author

Enalmada commented Nov 12, 2018

Ah yes, good idea. Thanks for telling me which one, that was very helpful. Tests do not seem to work on windows for me but I was able to WSL them following this: https://github.com/zeit/next.js/blob/canary/contributing.md

Unfortunately the tests seem to fail on CircleCI but pass fine on my local machine:
image
I am sure that CircleCI is correct but it is just so strange I get passing. If you could help me understand why you think the test is failing it might help me figure out why it is passing on my machine.

@Enalmada
Copy link
Contributor Author

Thanks for your help. I will spend some time figuring out how to get my tests working right.

@timneutkens timneutkens merged commit 4ce095d into vercel:canary Nov 13, 2018
@timneutkens
Copy link
Member

Great PR @Enalmada!

@jclem
Copy link

jclem commented Nov 13, 2018

Thank you @timneutkens and @Enalmada! Can't wait to see this in a release 🎉

@timneutkens
Copy link
Member

@jclem didn't know you were running into this at GitHub 😄 Feel free to DM me on spectrum/twitter if you run into specific issues.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add crossorigin attribute to NextScript
3 participants