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

Call stack errors when Astro component props are changed repeatedly in MDX files #4533

Closed
1 task
brycewray opened this issue Aug 29, 2022 · 21 comments · Fixed by vitejs/vite#10401
Closed
1 task
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope)

Comments

@brycewray
Copy link
Contributor

What version of astro are you using?

1.1.1

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

None

What package manager are you using?

npm

What operating system are you using?

macOS 12.5.1

Describe the Bug

Repeatedly editing the props supplied to an Astro (.astro) component imported into an MDX file causes a “maximum call stack size exceeded” error, often regarding String.replace. For example:

RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)

Although I first thought it was because I was accessing a Twitter API in some erroneous manner, @jasikpark helped me see that, even with the component reduced to only returning results from Math.random() (and, thus, the props are meaningless), the problem persists.

Note that I cannot reproduce the example on Stackblitz, so instead I will link to my own repo with a sample component and file...
Test component = https://github.com/brycewray/astro-site/blob/main/src/components/STweet.astro
Test MDX file = https://github.com/brycewray/astro-site/blob/main/src/pages/posts/2022/08/tweet-test.mdx

Link to Minimal Reproducible Example

[See above for info]

Participation

  • I am willing to submit a pull request for this issue.
@jasikpark
Copy link
Contributor

@jasikpark
Copy link
Contributor

Not able to see a reproduction of the bug in https://stackblitz.com/github/brycewray/astro-site/?file=README.md

@jasikpark
Copy link
Contributor

Seeing if https://codesandbox.io/s/github/brycewray/astro-site is different

@jasikpark
Copy link
Contributor

repro screenshot:

image

open the tweet-test.mdx post and trigger HMR at least 9-10 times by editing the STweet props and saving until you see the String.replace or module resolve stack overflow

@brycewray
Copy link
Contributor Author

This appears to be because, in a Post.astro layout, I was using await Astro.glob() to provide “previous page”/“next page” data to each post. With that glob reference removed from the layout, this issue doesn’t occur. This therefore doesn’t appear to be, legitimately, an Astro issue, so I am closing it.

At the suggestion of @jasikpark, this is now in the hands of the Vite Discord:
https://discord.com/channels/804011606160703521/814182556068085760/1013812534819692656

@jasikpark
Copy link
Contributor

well, it's still a direct astro feature, right? so it deserves issues for both projects 🤷

@brycewray brycewray reopened this Sep 2, 2022
@brycewray
Copy link
Contributor Author

Thought about what @jasikpark said above and decided he was right. Whether this is an edge case or something broader, it probably does need somebody to look at it when time allows.

@jasikpark
Copy link
Contributor

Is there a vite repo issue to track w/ it?

@brycewray
Copy link
Contributor Author

brycewray commented Sep 2, 2022

@jasikpark No. Actually didn’t think about that, especially after it never got any traction in the Vite Discord.

Edit: Wonder if this is related at all to vitejs/vite#9713 ...?

@jasikpark
Copy link
Contributor

This could be Astro specific as well, since the ability to import MD / MDX is an Astro-built Vite plugin, right?

@brycewray
Copy link
Contributor Author

This could be Astro specific as well, since the ability to import MD / MDX is an Astro-built Vite plugin, right?

Hope so. I’d be more optimistic about a fix in that case, although I gather at this point mine is an edge case since I don’t see reports of others having similar issues with Astro.glob() and the like.

@Princesseuh Princesseuh added feat: hmr Related to HMR (scope) - P4: important Violate documented behavior or significantly impacts performance (priority) labels Sep 23, 2022
@RobertAKARobin
Copy link

Possibly related: #4894

@brycewray
Copy link
Contributor Author

Possibly related: #4894

@RobertAKARobin Interesting and somewhat similar, although different from my issue since this happened for me without any other errors appearing prior to the call stack error.

@RobertAKARobin
Copy link

No other errors occur for me either.

@bluwy
Copy link
Member

bluwy commented Oct 10, 2022

Looks like https://github.com/brycewray/astro-site/blob/main/src/pages/posts/2022/08/tweet-test.mdx is missing and the repo is archived now, but I'm hitting a different issue with the repo that I'm getting JS heap errors on startup.

I generated a 1GB heap snapshot, but Chrome isn't loading it after 10mins. Marking this as blocked for now.

Is there a smaller repro too?

@bluwy
Copy link
Member

bluwy commented Oct 10, 2022

Ah alright I figured out where the cause is thanks to this screenshot. It's because of Vite hooking into Node's internal resolve, and it running in parallel causes the hook to be recursive. There was also prior discussion with a solution at https://discord.com/channels/804011606160703521/930868117234647092/930868121118584913. Making a fix on Vite's side.

@brycewray
Copy link
Contributor Author

Looks like https://github.com/brycewray/astro-site/blob/main/src/pages/posts/2022/08/tweet-test.mdx is missing and the repo is archived now, but I'm hitting a different issue with the repo that I'm getting JS heap errors on startup.

I generated a 1GB heap snapshot, but Chrome isn't loading it after 10mins. Marking this as blocked for now.

Is there a smaller repro too?

@bluwy I can both un-archive the repo and restore the missing file if that would help. Let me know.

@bluwy
Copy link
Member

bluwy commented Oct 10, 2022

No problem, I've made a fix at vitejs/vite#10401 👍

@brycewray
Copy link
Contributor Author

brycewray commented Oct 10, 2022

No problem, I've made a fix at vitejs/vite#10401 👍

@bluwy Outstanding! Thank you. 👏 👏 👏 👏 🎉 🎉 🎉 🎉

@jasikpark Realistically speaking: once this fix goes through on Vite’s site, how long would it take before this is reflected in Astro? Is there coding on that project’s side which will be necessary to implement a Vite-side fix like this? (I’ll also be curious as to whether this might fix the slow-as-molasses experience I’ve had with editing MDX files, since they all were wrapped in layouts using Astro.glob().)

@bluwy
Copy link
Member

bluwy commented Oct 10, 2022

@brycewray the Vite fix will be going into Vite 3.2 which will take a week or so before reaching stable, which Astro would then bump the version to support it. Once the fix is published in [email protected] you can also start using it with npm overrides. There shouldn't be any change needed in Astro.

@brycewray
Copy link
Contributor Author

@bluwy Thanks for that info, and thanks again for the fix!

cc: @jasikpark

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) feat: hmr Related to HMR (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants