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

fix: context improvements #921

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

raymondmuller
Copy link
Collaborator

@raymondmuller raymondmuller commented Nov 18, 2022

Description

This includes some better support / improvements for context setting.

Parser changes

  • It allows you to set a 'string' as the key directly (updated the type)
  • Allow passing an object or variable directly, including state

Fixes for the Vue generator.

  • Not appending .value when passing a ref as that keeps the provided context reactive
  • Prevent appending .value to object properties

Fixes for the Svelte generator

  • Change the order in which we render the json. Data string (e.g. let disabled = true) should be there before we setContext, as we might set the context using those variables)

Examples:

import { setContext, useState } from '@builder.io/mitosis';
export default function MyComponent(props) {
  const [disabled, setDisabled] = useState(false);
  setContext('disabled', { disabled });
  return <h1>Hello World</h1>;
}
import { setContext, useState } from '@builder.io/mitosis';
export default function MyComponent(props) {
  const [disabled, setDisabled] = useState(false);
  setContext('disabled', disabled);
  return <h1>Hello World</h1>;
}
import { setContext, useState } from '@builder.io/mitosis';
export default function MyComponent(props) {
  setContext('hello', props.world);
  return <h1>Hello World</h1>;
}

Note:
Doesn't seem to work for Solid, but we should probably fix that in the generator?

@vercel
Copy link

vercel bot commented Nov 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
mitosis-fiddle ✅ Ready (Inspect) Visit Preview Nov 18, 2022 at 3:55PM (UTC)

@raymondmuller raymondmuller changed the title Context improvements fix: context improvements Nov 18, 2022
Copy link
Contributor

@samijaber samijaber left a 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.

NOTES:

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?

@raymondmuller
Copy link
Collaborator Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants