Skip to content

document behavior of script tags in JSX expressions#3111

Merged
sarah11918 merged 7 commits into
mainfrom
jsx-inline-script
May 3, 2023
Merged

document behavior of script tags in JSX expressions#3111
sarah11918 merged 7 commits into
mainfrom
jsx-inline-script

Conversation

@MoustaphaDev
Copy link
Copy Markdown
Member

@MoustaphaDev MoustaphaDev commented Apr 28, 2023

What kind of changes does this PR include?

  • New or updated content

Description

Closes withastro/astro#6905

Scripts inside JSX expressions are treated as inline scripts tags. I believe this is because the compiler doesn't extract these scripts because we wouldn't be able to know whether or not to include them in the head until the component template is run.

All these scripts will be inline scripts

{
    <script>
        alert('Hello Astro!');
    </script>
}
{
    false && <script>
       alert(''Unreachable")
    </script>
}
{
    true && <script>
        // will throw since inline scripts aren't processed to compile TS -> JS
        const name = document.getElementById('name') as HTMLInputElement;
    </script>
}

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 28, 2023

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 3dd0d60
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/6452b6dc5a0e70000898de18
😎 Deploy Preview https://deploy-preview-3111--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@MoustaphaDev MoustaphaDev requested a review from a team April 28, 2023 10:12
@MoustaphaDev MoustaphaDev added the add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. label Apr 28, 2023
@Princesseuh
Copy link
Copy Markdown
Member

I'm not sure why my specific review was asked for, but this looks good to me 👍

I wonder if there would be value in documenting workarounds, though those also come with caveats

@MoustaphaDev
Copy link
Copy Markdown
Member Author

I'm sorry if the ping was too broad, but glad to hear it looks good

@MoustaphaDev MoustaphaDev removed the request for review from a team April 28, 2023 19:27
@sarah11918
Copy link
Copy Markdown
Member

Give me some time to think about this, @MoustaphaDev , because you've changed the focus of the note now away from the idea of adding an attribute to a script tag (and them what happens when you do that), and it now is starting to list different things that happen to have the same effect / end result, putting the focus on the things that make scripts not bundled.

It's a subtle difference, but it really does change the original note's purpose. So, I've been thinking about how to best include this new information.

@MoustaphaDev
Copy link
Copy Markdown
Member Author

MoustaphaDev commented May 1, 2023

I understand, I've seen the use of two consecutive admonitions in some pages of the docs. We can document this addition in a “warning” block maybe?

Comment thread src/content/docs/en/guides/client-side-scripts.mdx Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

add new content Document something that is not in docs. May require testing, confirmation, or affect other pages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Script in JSX get stripped out

4 participants