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

[v5] React .provide support #3947

Merged
merged 12 commits into from
May 10, 2023
Merged

[v5] React .provide support #3947

merged 12 commits into from
May 10, 2023

Conversation

davidkpiano
Copy link
Member

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2023

🦋 Changeset detected

Latest commit: ab5bb51

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@xstate/react Major

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 6, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ab5bb51:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@ghost
Copy link

ghost commented Apr 6, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

TMachine['__TResolvedTypesMeta']
> extends false
? {
options: InternalMachineImplementations<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<Provider machine={someMachine.provide()} /> is workable but i think that it makes it a little bit awkward because we are repeating someMachine here - we already given it to create the Provider , it's available internally as a closure, types are bound to it === it looks like a chore that we still require it to be used directly here.

The best idea I have so far to solve this is for the behavior to somehow declare it's available options. This way the interpreter-level stuff could accept those options in a generic way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smth like this could work

<Context.Provider machine={machine => machine.provide({})}>{null}</Context.Provider>

That's quite a bit of machine in here though 🤔

@@ -32,40 +29,21 @@ export function useIdleInterpreter(
) {
const [initialMachine] = useState(machine);

if (getMachine !== initialMachine) {
if (getMachine.config !== initialMachine.config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, .config is super machine-specific thing. If we want to make useInterpret to accept just any actor then it should be possible to check smth like this on just any actor type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To solve this we could do this:

const config = () => Promise.resolve('')
const behavior = fromPromise(config)
behavior.config === config

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I'll think about how we can generalize this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to 4659f45 - let me know what you think. Might need help on this

@Andarist Andarist merged commit 5fa3a0c into next May 10, 2023
3 checks passed
@Andarist Andarist deleted the v5/react-prefer-provide branch May 10, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants