-
Notifications
You must be signed in to change notification settings - Fork 16
fix(mdxish): jsx braces not evaluating #1256
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
base: next
Are you sure you want to change the base?
fix(mdxish): jsx braces not evaluating #1256
Conversation
| // These are not exhaustive, but are a good starting point | ||
| // We probably want to just use a library that'll load most of the common operations for us | ||
| export const DEFAULT_JSX_CONTEXT: JSXContext = { | ||
| uppercase: (value: string) => value.toUpperCase(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Struggling to understand why we're constructing a special context with these operations? If I understand correctly ES language features are already available when we eval the expressions. And I don't believe these functions are available in strict MDX so why/how would a user even know they exist?
Instead of Hello {uppercase("world")} I would instead write Hello {"world".toUpperCase()}
and instead of {add(1, 2)} I would write {1 + 2}
and so on...
Is there a requirement I'm missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you are right, apologies, I've found that the issue was with another code that was skipping evaluation if no function context / parameter names are provided. We don't need to define the default contexts for evaluating, so that code check is actually not needed.
Added tests for the string functions you suggested in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using mix instead of mdxish since mix returns a string already (mix is basically stringified mdxish)
| const contextKeys = Object.keys(context); | ||
| const contextValues = Object.values(context); | ||
|
|
||
| // If no context provided, leave expression as-is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problematic check that is now removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving some things around for cleanliness
🧰 Changes
🧬 QA & Testing