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

Update script.md #37084

Closed
wants to merge 2 commits into from
Closed

Update script.md #37084

wants to merge 2 commits into from

Conversation

valse
Copy link
Contributor

@valse valse commented May 21, 2022

The Script with the beforeInteractive strategy must be in the _document.js Head otherwise it fails to load.

The Script with the `beforeInteractive` strategy must be in the `_document.js` `Head` otherwise it fails to load.
Run prettier
@ijjk ijjk requested a review from housseindjirdeh May 22, 2022 22:24
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.

<Script> should definitely not be in the custom components so this sounds more like a bug that should be investigated. Could you open an issue with a reproduction / test case?

@icyJoseph
Copy link
Contributor

icyJoseph commented May 24, 2022

Hi,

I think this was fixed on #37000 - Tried to jump at fixing it myself, but the tests passed... so I checked git blame and found that PR 😄

Though it does seem like #37000 expects the Script to be inside Head.

@valse
Copy link
Contributor Author

valse commented May 25, 2022

Though it does seem like #37000 expects the Script to be inside Head.

Hi, exactly... I'm using it and I had to move it inside Head so why I made this PR.

Copy link
Collaborator

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Sorry for the confusion folks, the newer behavior of <Script> that @janicklas-ralph added does make it so that it needs to be inserted in the document (but not in page components).

I'll submit an update to remove this restriction. We can merge this PR in the meantime or hold off (I'm fine either way), but if it does get merged, I'll update the docs 👍

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

As discussed with @housseindjirdeh and @janicklas-ralph in Slack this restriction isn't desired and we will patch this in a follow-up PR so that inserting scripts in Head isn't required so I'm gonna close this but thanks for raising this PR!

@ijjk ijjk closed this May 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 28, 2022
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.

5 participants