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

inject script tag in mdx content #6327

Closed
1 task done
y-nk opened this issue Feb 22, 2023 · 11 comments · Fixed by #6459
Closed
1 task done

inject script tag in mdx content #6327

y-nk opened this issue Feb 22, 2023 · 11 comments · Fixed by #6459
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: mdx Issues pertaining to `@astrojs/mdx` integration

Comments

@y-nk
Copy link
Contributor

y-nk commented Feb 22, 2023

What version of astro are you using?

latest

Are you using an SSR adapter? If so, which one?

no

What package manager are you using?

npm@8

What operating system are you using?

macOS@latest

Describe the Bug

Problem

When trying to add a script tag in an mdx file, the script tag isn't parsed properly.

Without valid JSX syntax, as expected the content gets parsed as html, quotes are properly respected:

<!-- input -->
<script>
window.alert('foo')
</script>

<!-- output -->
<script><p>window.alert('foo')</p></script>

With valid JSX syntax, surprisingly the content of the tag gets escaped:

<!-- input -->
<script>{`window.alert('foo')`}</script>

<!-- output -->
<script>window.alert(&#39;foo&#39;)</script>

I also tried is:raw, is:inline and the client:* combinations without success.

Goal

To inject client only small scripting without having anything else to do than <script>. I know that remark-mdx would replace script tags to div, but since it works here... why not push it to the max?

Workaround

Add a framework integration and rely on them by creating a third party script component such as:

export default function Script({ value }: { value: string }) {
  return <script dangerouslySetInnerHTML={{ __html: value }} />;
}

...but i wished i didn't have to rely on [React/Vue/etc...] integration to do this.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-vuknlr?file=src/pages/index.mdx&on=stackblitz

Participation

  • I am willing to submit a pull request for this issue.
@natemoo-re natemoo-re added the - P4: important Violate documented behavior or significantly impacts performance (priority) label Feb 23, 2023
@natemoo-re
Copy link
Member

Sorry for the frustration! I'm curious if set:html works? That's our equivalent to dangerouslySetInnerHTML

@natemoo-re natemoo-re added the needs response Issue needs response from OP label Feb 23, 2023
@y-nk
Copy link
Contributor Author

y-nk commented Feb 24, 2023

@natemoo-re after testing, the set:html works but then it'll still be a plain string, so it's not that different from the workaround i've proposed (with the React component).

I made a rewrite in Astro with a little enhancement. It feels a bit more like is:inline now. The DX is much better but still "not 🐐"-rated, no HMR (since i'm using readFileSync) and bundling is still not possible... so it's pretty limited.

Here's for posterity:

---
import { readFileSync } from "fs"
import { dirname, join } from "path"

const { src, url } = Astro.props

export type Props = {
  src: string
  url?: string
}

const dir = url ? dirname(new URL(url).pathname) : process.cwd()
const code = readFileSync(join(dir, src), 'utf-8')
---
<script set:html={code} />

to be used as:

import Script from '~/components/Script.astro'

<Script src='./client-script.js' url={import.meta.url} />

it's ok for small scripts especially if you're loading client-side umd libraries from unpkg, but i really do think there's something to do here to enable bundling, minification, etc...

EDIT: I've used fs instead of dynamic imports bc like every other bundler, vite can't bundle dynamic imports so i figured "why bother"

@mardybardy
Copy link

mardybardy commented Feb 24, 2023

Cross posting from discord in case people come here from the web:
This seems to work.

Create an astro component called Scriptify that just contains a slot:

// components/Scriptify.astro

---
---
<slot />

Then in your MDX:
// MyPost.mdx

---
title: MyPost
---
import Scriptify from "@components/Scriptify"

<Scriptify>
    <script>
        const a = 3;
        const b = 2;
        console.log(a * b);
    </script>
</Scriptify>

You should see "6" in your browser console.

@y-nk
Copy link
Contributor Author

y-nk commented Feb 27, 2023

@feraleyebrows sadly it seems not to work: https://stackblitz.com/edit/github-4sr41n

Have i missed something from your solution?

@mardybardy
Copy link

@y-nk pushed an example repo here because stackblitz was being a pain. needed to add some curly braces to get it to work across multiple lines e.g:

<Scriptify>
{
    <script>
            const a = 3; 
            const b = 2; 
            const c = 10; 
            const d = 100; 
            console.log(a * b);
    </script>
}
</Scriptify>

https://github.com/feraleyebrows/script-in-mdx-astro-example

@y-nk
Copy link
Contributor Author

y-nk commented Mar 1, 2023

I've made long comments on the Discord thread to explain what happens when we write this ↑ and why it's not a suitable solution.

Although, it was good to mention it because it made me dig deeper into Astro's internal and probably found the bug.

It seems that style and script tags should not be escaped by default. I know it's a security concern but imho it should be implemented differently. I understand it's a good feature to have but it should not be blocking from using plain working html.

I've isolated the culprit here.

case typeof vnode === 'string':
return markHTMLString(escapeHTML(vnode));

where it seems that script tags are escaped by default.

How to reproduce in stackblitz:

  1. CTRL+C in the console to stop the server
  2. open node_modules/astro/dist/runtime/server/jsx.js
  3. Modify the code above to remove the escapeHTML call
  4. save the file and run the server again with npm run dev

When doing so, the only working example will be:

<script>{`
  window.alert('hello world with jsx')
`}</script>

and that's to be expected because it's regular JSX syntax (even though it's painful)

@bluwy bluwy added pkg: mdx Issues pertaining to `@astrojs/mdx` integration and removed needs response Issue needs response from OP labels Mar 7, 2023
@bluwy bluwy self-assigned this Mar 8, 2023
@bluwy
Copy link
Member

bluwy commented Mar 8, 2023

I've made #6459, which should fix the rendering issue you pointed above @y-nk (Thanks for debugging it). But it won't support directly using <script> tags and writing JavaScript within it yet. Mostly because most MDX implementations don't support it, and reaching feature parity with Astro templates isn't possible in some case. But an RFC for that is welcome if there's enough interest of a feature like this, and we can investigate further.

@y-nk
Copy link
Contributor Author

y-nk commented Mar 8, 2023

thanks a lot @bluwy :)

if that's any help for people like me, i'm softly planning to publish a remark plugin which will patch the remark parser and add curly braces and backticks to script and style tags. it's a quick hack but i'm not that confident yet to modify the great remark mdx 😂

@bluwy
Copy link
Member

bluwy commented Mar 8, 2023

I actually was planning the exact same hack if we were to implement support too 🙈 But since that deviates from Astro's semantics for script and style tags, I've decided not to for now. If you do get around creating that plugin, I think it's great to experiment how well that works first!

@hilmanski
Copy link

@y-nk pushed an example repo here because stackblitz was being a pain. needed to add some curly braces to get it to work across multiple lines e.g:

<Scriptify>
{
    <script>
            const a = 3; 
            const b = 2; 
            const c = 10; 
            const d = 100; 
            console.log(a * b);
    </script>
}
</Scriptify>

https://github.com/feraleyebrows/script-in-mdx-astro-example

Thanks! It doesn't work when we have > (greater than sign)
sample:

    const boxes = document.querySelectorAll('.boxes-demo-1 > div');

error
Could not parse expression with acorn: Unexpected token >. Did you mean > or {">"}?

@hilmanski
Copy link

[Update]
It worked using this method #6327 (comment)
wrapping {...} inside script tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: mdx Issues pertaining to `@astrojs/mdx` integration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants