Fixed a type compatibility with Svelte's readables #3063

Andarist · 2022-02-18T17:48:13Z

changeset-bot · 2022-02-18T17:48:15Z

🦋 Changeset detected

Latest commit: c4ed71e

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

This PR includes changesets to release 1 package

Name	Type
xstate	Patch

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

Andarist · 2022-02-18T17:49:17Z

.github/workflows/ci-checks.yml

@@ -0,0 +1,12 @@
+name: 'CI checks'
+runs:
+  using: 'composite'


I had to add svelte-check to our CI jobs here and I've thought that it would be nice to experiment with those composite actions - we'll see how it goes, it's the first time when I've attempted to use this.

ghost · 2022-02-18T17:49:19Z

CodeSee Review Map:

Review in an interactive map

View more CodeSee Maps

Legend

codesandbox-ci · 2022-02-18T17:49:31Z

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 c4ed71e:

Sandbox	Source
XState Example Template	Configuration
XState React Template	Configuration

Andarist · 2022-02-18T17:49:39Z

package.json

@@ -15,9 +15,7 @@
    "test": "jest",
    "test:core": "npm test --prefix packages/core",
    "dev": "node ./scripts/dev.js",
-    "ci": "npm run build && npm run test",


from what I can tell this wasn't actually used by anything

Andarist · 2022-02-18T17:50:18Z

package.json

    "changeset": "changeset",
-    "prerelease": "npm run build && npm run test",


"pre*" scripts dont work with all package managers, so I've decided to just move this directly to a github action

Andarist · 2022-02-18T17:51:05Z

packages/core/rollup.config.js

@@ -4,33 +4,6 @@ import { terser } from 'rollup-plugin-terser';
 import rollupReplace from 'rollup-plugin-replace';
 import fileSize from 'rollup-plugin-filesize';

-const stripSymbolObservableMethodPlugin = ({ types: t }) => {


It's just no longer needed since I've figured out how to assign symbolObservable to a "variable" and maintain the Symbol.observable type at the same time

Andarist · 2022-02-18T18:13:42Z

packages/core/src/utils.ts

@@ -529,15 +529,18 @@ export function isObservable<T>(value: any): value is Subscribable<T> {
  }
 }

-export const symbolObservable = (() =>
+const symbolObservable = (() =>


Yes, what happens in this file is weird and I don't have much on my defense... except... this TS playground

Andarist · 2022-02-18T18:14:45Z

packages/core/test/types.test.ts

@@ -386,3 +387,23 @@ describe('Typestates', () => {
    expect(failed).toEqual({ result: none, error: 'oops' });
  });
 });
+
+describe('interpreter', () => {
+  it('should be convertable to Rx observable', () => {


an explicit test covering for changes from: https://github.com/statelyai/xstate/pull/2371/files

Andarist · 2022-02-18T18:15:32Z

packages/xstate-svelte/test/interpreterAsReadable.svelte

@@ -0,0 +1,19 @@
+<script lang="ts">
+  // this file acts as a type test
+  import { createMachine, interpret } from 'xstate';


this is a new test for what is being fixed in this PR

Andarist · 2022-02-18T18:26:14Z

packages/core/src/types.ts

+export interface InteropSubscribable<T> {
+  subscribe(observer: Observer<T>): Subscription;
+}


In a way, this breaks the previous contract exposed under Symbol.observable method. However, this is the only signature that is valid from Rx's PoV. The InteropObservable returns Subscribable<T>:
https://github.com/ReactiveX/rxjs/blob/67cb317a7a6b9fdbd3d2e8fdbc2ac9ac7e57179c/api_guard/dist/types/index.d.ts#L321-L323
and Subscribable is defined as follows:
https://github.com/ReactiveX/rxjs/blob/67cb317a7a6b9fdbd3d2e8fdbc2ac9ac7e57179c/api_guard/dist/types/index.d.ts#L690-L692

So, in fact, this interface only accepts observers - it doesn't accept separate listener functions for next, error and complete.

By implementing this change and using this as an explicit return type of the Symol.observable method on the Interpreter class we ensure that it's compatible with the Rx's signature for this:

xstate/packages/core/src/interpreter.ts

Lines 1369 to 1373 in d912a8d

public [symbolObservableRef.symbol](): InteropSubscribable<

State<TContext, TEvent, TStateSchema, TTypestate, TResolvedTypesMeta>

> {

return this;

}

Thanks to that we can revert the reordering of the subscribe's overloads from this PR and fix the issue with Svelte's readables. This is because both libraries are using conditional types to match the input argument and to "extract" the emitted type and conditional types are only seeing the last overload.

So, in a sense, this is a hack and might break any minute... cause the whole thing is fitting both libraries thanks to the alignment of the stars and not because this is built on super solid principles. We trick Rx by only exposing a single overload with the observer as the parameter for its conditional type from the Symbol.observable method and we trick Svelte by reordering overloads so it only sees the one with callbacks parameters.

Andarist · 2022-02-18T18:27:07Z

packages/core/src/interpreter.ts

+        const observer = toObserver(next);
+        listeners.add(observer.next);


this is a fix from the perspective of the Symbol.observable - since that method is accepting observers we convert the received callback (or observer) to an observer

Andarist · 2022-02-18T19:39:33Z

CI is expected to fail right now. It rightfully reports errors in some svelte files related to state.matches typings - this is a new error from CI's perspective because we actually type-check those svelte files now. The error was there all along, it just slipped through the cracks because CI was not verifying those files.

I'll look into fixing this problem and I think it's OK to wait with this PR a little bit before I do so.

Andarist · 2022-02-19T09:52:40Z

The CI should get unblocked in a sec since I've landed #3065

Andarist commented Feb 18, 2022

View reviewed changes

Andarist force-pushed the andarist/fix-2533 branch from 4e22e7d to d912a8d Compare February 18, 2022 18:11

Andarist commented Feb 18, 2022

View reviewed changes

Andarist force-pushed the andarist/fix-2533 branch 3 times, most recently from b8b4a79 to f5485bd Compare February 18, 2022 19:30

Andarist mentioned this pull request Feb 18, 2022

Version 4.23.0 breaks typing of "$" prefixed service inside svelte component #2533

Closed

Fixed a type compatibility with Svelte's readables

c4ed71e

Andarist force-pushed the andarist/fix-2533 branch from f5485bd to c4ed71e Compare February 19, 2022 09:51

Andarist marked this pull request as ready for review February 19, 2022 09:52

davidkpiano approved these changes Feb 19, 2022

View reviewed changes

Andarist merged commit c826559 into main Feb 19, 2022

Andarist deleted the andarist/fix-2533 branch February 19, 2022 13:57

github-actions bot mentioned this pull request Feb 19, 2022

Version Packages #3054

Merged

wlee221 mentioned this pull request Feb 23, 2022

Bug: xstate@4.30.2 is incompatible with typescript@<4.3.0 #3081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed a type compatibility with Svelte's readables #3063

Fixed a type compatibility with Svelte's readables #3063

Andarist commented Feb 18, 2022

changeset-bot bot commented Feb 18, 2022 •

edited

Loading

Andarist Feb 18, 2022

ghost commented Feb 18, 2022 •

edited by ghost

Loading

codesandbox-ci bot commented Feb 18, 2022 •

edited

Loading

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist Feb 18, 2022

Andarist commented Feb 18, 2022

Andarist commented Feb 19, 2022

		"changeset": "changeset",
		"prerelease": "npm run build && npm run test",

	public [symbolObservableRef.symbol](): InteropSubscribable<
	State<TContext, TEvent, TStateSchema, TTypestate, TResolvedTypesMeta>
	> {
	return this;
	}

		const observer = toObserver(next);
		listeners.add(observer.next);

Fixed a type compatibility with Svelte's readables #3063

Fixed a type compatibility with Svelte's readables #3063

Conversation

Andarist commented Feb 18, 2022

changeset-bot bot commented Feb 18, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

ghost commented Feb 18, 2022 • edited by ghost Loading

CodeSee Review Map:

Legend

codesandbox-ci bot commented Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Feb 18, 2022

Andarist commented Feb 19, 2022

changeset-bot bot commented Feb 18, 2022 •

edited

Loading

ghost commented Feb 18, 2022 •

edited by ghost

Loading

codesandbox-ci bot commented Feb 18, 2022 •

edited

Loading