-
Notifications
You must be signed in to change notification settings - Fork 572
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: context improvements #921
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Blocker - React & SolidJS
There's one big issue with allowing Contexts to be defined by strings instead of Context objects: certain frameworks (like React and SolidJS) need a Context object to be created first, and then we use that object reference to both read and set the context value:
// my-context.ts
import React from 'react';
export default React.createContext({ foo: 'bar' })
// parent.tsx
import React from 'react';
import MyContext from './my-context';
export default function Parent(props) {
return (
<MyContext.Provider value={{ foo: '123' }}>
<child />
<MyContext.Provider>
)
}
// child.tsx
import React from 'react';
import MyContext from './my-context';
export default function Child(props) {
const myContextValue = React.useContext(MyContext);
return (
<div> {myContextValue.foo} </div>
)
}
If we want to be able to use strings as context references, it will require extra work for React & SolidJS (not sure if there are other web frameworks that also need a special Context object to be created first)
Solution
- every time the Solid/React generators stumble on a
string
key context, they should- return a separate context file for that context key. Those files will be one-liner, like
export default React.createContext(undefined)
, and will be created in a special directory, e.g.output/react/mitosis-context/{key}.js
. If another file already had that key in its code, then the context file will already exist, and nothing needs to be done. - add an import to that special context file, and use the imported object instead.
- return a separate context file for that context key. Those files will be one-liner, like
NOTES:
- This will be possible once feat: Generators can now emit more than one output file. #865 is merged, as it will allow generators to output multiple files. We can add a
context
output type.
I think we'll want to make sure to do the above first, as it will be rather confusing to not have these string contexts work in React & SolidJS.
PS: I know that using a string key is a lot less work, but worth noting that it's also a lot less safe: the context object approach guarantees that the components reference the exact same context, and we can provide type annotations, whereas the string
approach means that:
- typos can easily happen
- since there is no context object being references between setters and getters, the types can't flow or be in-sync unless extra manual work is done.
Part of me wonders if we should even bother with this, or just stick with the context object approach for now. WDYT?
Thanks for reviewing. I would prefer to support this to be honest. As far as I am aware this is pretty standard way of setting the context in Svelte and even though I do get your concerns, I always hoped for Sveltosis to stay as close to the original Svelte syntax as possible. What about solving this at a parser level? What I mean by that, is we can generate any JSON we want based on the input and instead of supporting 'strings' as a key in each and every generator, we could simply convert it to the required format + have the context in a separate file (once #865 is merged). |
Description
This includes some better support / improvements for context setting.
Parser changes
Fixes for the Vue generator.
Fixes for the Svelte generator
let disabled = true
) should be there before we setContext, as we might set the context using those variables)Examples:
Note:
Doesn't seem to work for Solid, but we should probably fix that in the generator?