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
fix(renderer): omitting internal symbol from mdx props #10813
Open
Xetera
wants to merge
1
commit into
withastro:main
Choose a base branch
from
Query-Doctor:fix-renderer-internal-symbol
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 6d0645d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: integration
Related to any renderer integration (scope)
pkg: astro
Related to the core `astro` package (scope)
labels
Apr 18, 2024
Xetera
changed the title
fix(renderer): omitting internal symbol from user component props
fix(renderer): omitting internal symbol from mdx props
Apr 18, 2024
matthewp
reviewed
Apr 19, 2024
!bench render |
This comment was marked as outdated.
This comment was marked as outdated.
Xetera
force-pushed
the
fix-renderer-internal-symbol
branch
2 times, most recently
from
April 19, 2024 12:47
3b7c092
to
b57b8cf
Compare
Xetera
force-pushed
the
fix-renderer-internal-symbol
branch
from
April 19, 2024 13:43
b57b8cf
to
6d0645d
Compare
!bench render |
PR BenchmarkRender
Main BenchmarkRender
|
bluwy
approved these changes
Apr 23, 2024
ematipico
approved these changes
Apr 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
pkg: astro
Related to the core `astro` package (scope)
pkg: integration
Related to any renderer integration (scope)
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've noticed that when using the following code in an mdx file with a content collection, everything works fine if I have a properly compiling page
But if there's an error anywhere in the codebase after a react dependency is being used, let's say
we're greeted with an error that makes no sense
But if the error happens earlier in the code like
then we get the expected error
Here's a minimal reproducible example: https://codesandbox.io/p/devbox/magical-hoover-phk2cv?file=%2Fastro.config.mjs%3A9%2C2
After some digging, I found that the problem happens because the symbol
gets passed down to the user's component.
The weird error messages even go away if we manage to get rid of that symbol in the mdx outside the compiler before the react component is rendered, like:
While trying to unknowningly recreate the above example using an old astro version before v4.5.9, I realized that all react mdx components used to have this issue even without
{...props}
being spread.After updating astro to latest, I traced this problem getting partially fixed with this commit 627e47d.
I'm guessing it has something to do with
Skip.symbol
always being passed in, but the new symbol is also always being passed in. I guess I'm not familiar enough with the astro compiler internals to know why that was always erroring and the above commit made it only break when props is spread. Oh well.Here's the same problem happening with v4.5.8 without any props getting passed to the component in the mdx file in case you're curious https://codesandbox.io/p/devbox/compassionate-hypatia-glvdg5?file=%2Fastro.config.mjs
Changes
I'm not 100% sure what part of the process
vnode.type()
exactly represents. Omitting the symbol here seems to magically solve the problem, but I can't really explain why a symbol being passed to a component would cause React to be null. Since this is a build error, I can't compare bundle outputs either to see what exactly is being changed either.Since this function continues to recurse with the value of
vnode.props
, I had to spread the symbol out of it instead of mutating.Testing
I added a regression test for the user-facing part of the problem which is that error messages get messed up during build errors. I don't think it would be valuable to check for any specific symbol in the mdx here
Docs
Shouldn't need a documentation change for a bug fix