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] Remove observable replay #3877

Merged
merged 11 commits into from
Mar 23, 2023

Conversation

davidkpiano
Copy link
Member

@davidkpiano davidkpiano commented Mar 4, 2023

This PR makes actor.subscribe(...) a hot observable; it will no longer emit the current snapshot, which can be retrieved synchronously via actor.getSnapshot().

@linear
Copy link

linear bot commented Mar 4, 2023

STA-498

@changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2023

🦋 Changeset detected

Latest commit: ddf64b6

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

This PR includes changesets to release 1 package
Name Type
xstate 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

@davidkpiano davidkpiano changed the base branch from main to next March 4, 2023 15:57
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 4, 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 ddf64b6:

Sandbox Source
XState Example Template Configuration
XState React Template Configuration

@davidkpiano
Copy link
Member Author

@GoldingAustin Do you know what can be done here to make @xstate/solid work with the actor (service) as a hot observable? We can still get the initial state from actor.getSnapshot().

@tivac
Copy link
Contributor

tivac commented Mar 4, 2023

I can't look at the Linear ticket but would love to understand the reasoning behind this change. I'm used to svelte store style observables so .subscribe() immediately sending the current value was exactly how I expected it to work.

@Andarist
Copy link
Member

Andarist commented Mar 4, 2023

Often u might be interested to listen for a change. This might be different from UI-related needs where you might want to render a retrieved value immediately though. The point is that if you want to listen for a change then it's really quirky to ignore the first emitted value but when you want to combine change+current value... that is more straightforward to do.

@tivac
Copy link
Contributor

tivac commented Mar 4, 2023

Thanks for explaining the reasoning @Andarist, I really appreciate the insight. I'm firmly over here in UI-land so this isn't a change I'm excited about but at least I understand it now.

@davidkpiano davidkpiano changed the title [v5] Make observables hot [v5] Remove observable replay Mar 4, 2023
@davidkpiano
Copy link
Member Author

Thanks for explaining the reasoning @Andarist, I really appreciate the insight. I'm firmly over here in UI-land so this isn't a change I'm excited about but at least I understand it now.

@tivac You're in luck; since @xstate/svelte implements stores like this:

  //  initial snapshot   vvvvvvvvvvvvvvvvvvvvv
  const state = readable(service.getSnapshot(), (set) => {
    return service.subscribe((state) => {
      if (state.changed) {
        set(state);
      }
    }).unsubscribe;
  });

I believe everything will still work the same, and you will have the initial snapshot available immediately regardless of these changes. It might even work better because you won't get a redundant update (two initial snapshots - one with .getSnapshot() and one upon subscribing).

@tivac
Copy link
Contributor

tivac commented Mar 5, 2023

I'm mostly subscribing to interpreters via xstate-component-tree these days, but that library will certainly be updated to work with v5 so it's not a huge deal.

@ghost
Copy link

ghost commented Mar 14, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@davidkpiano
Copy link
Member Author

It seems like the only failing tests are for @xstate/solid - @GoldingAustin can you help with this?

The context is that .subscribe will no longer receive the current snapshot; that needs to be read from actor.getSnapshot().

@GoldingAustin
Copy link
Contributor

I'll take a look at this today!

@GoldingAustin
Copy link
Contributor

It seems like the only failing tests are for @xstate/solid - @GoldingAustin can you help with this?

The context is that .subscribe will no longer receive the current snapshot; that needs to be read from actor.getSnapshot().

This will be a quick fix; I'll remove the from test suite. I can update the current tests to not use from or create a new function export from xstate-solid that emulates the past from functionality. Do you have any preferences on the direction?

@davidkpiano
Copy link
Member Author

It seems like the only failing tests are for @xstate/solid - @GoldingAustin can you help with this?
The context is that .subscribe will no longer receive the current snapshot; that needs to be read from actor.getSnapshot().

This will be a quick fix; I'll remove the from test suite. I can update the current tests to not use from or create a new function export from xstate-solid that emulates the past from functionality. Do you have any preferences on the direction?

I think the new function export would be the best direction (cc. @Andarist)

@Andarist
Copy link
Member

Yeah, I think that a new export is the best solution here - unless from is already easily replaceable with something. Isn't this mostly the same thing?

-const state = from(actor)
+const [state] = useActor(actor)

A notable difference is that useActor used the whole createImmutable under the hood... but that feels like an improvement, right?

@Andarist Andarist force-pushed the v5/sta-498-ontransition-vs-subscribe-distinction branch from 2ce6dbf to 3c1886f Compare March 23, 2023 15:14
…tions (#3898)

* Tweak changes related to dropping replaying of the last notifications

* Remove unused import

* Fixed Vue test

* remove debugger
@Andarist Andarist force-pushed the v5/sta-498-ontransition-vs-subscribe-distinction branch from 22a736e to 0333b2f Compare March 23, 2023 16:45
@Andarist Andarist force-pushed the v5/sta-498-ontransition-vs-subscribe-distinction branch from 82c6c98 to ddf64b6 Compare March 23, 2023 16:52
'xstate': major
---

Observing an actor via `actor.subscribe(...)` no longer immediately receives the current snapshot. Instead, the current snapshot can be read from `actor.getSnapshot()`, and observers will receive snapshots only when a transition in the actor occurs.
Copy link
Member

Choose a reason for hiding this comment

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

note for me - this might require a small tweak in the wording if we decide to go with #3912

@Andarist Andarist merged commit 1269470 into next Mar 23, 2023
@Andarist Andarist deleted the v5/sta-498-ontransition-vs-subscribe-distinction branch March 23, 2023 16:54
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

4 participants