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

Fixed a type compatibility with Svelte's readables #3063

Merged
merged 1 commit into from
Feb 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curvy-pears-cheat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'xstate': patch
---

Fixed a type compatibility with Svelte's readables. It should be possible again to use XState interpreters directly as readables at the type-level.
15 changes: 15 additions & 0 deletions .github/actions/ci-checks/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
name: 'CI checks'
runs:
using: 'composite'
steps:
- name: Build
run: yarn build
shell: bash

- name: Test
run: yarn test --silent
shell: bash

- name: Svelte Check
run: yarn --cwd packages/xstate-svelte svelte-check
shell: bash
12 changes: 12 additions & 0 deletions .github/actions/ci-setup/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
name: 'CI setup'
runs:
using: 'composite'
steps:
- name: Use Node.js 14.x
uses: actions/setup-node@v2
with:
node-version: 14.x

- name: Install Dependencies
run: yarn
shell: bash
20 changes: 2 additions & 18 deletions .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,7 @@ jobs:

runs-on: ubuntu-latest

strategy:
matrix:
node-version: [14.x]

steps:
- uses: actions/checkout@v2

- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v2
with:
node-version: ${{ matrix.node-version }}

- name: Install dependencies
run: yarn install

- name: Build
run: yarn build

- name: Test
run: yarn test --silent
- uses: ./.github/actions/ci-setup
- uses: ./.github/actions/ci-checks
13 changes: 2 additions & 11 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,8 @@ jobs:

steps:
- uses: actions/checkout@v2
with:
# This makes action fetch all Git history so that Changesets can generate changelogs with the correct commits
fetch-depth: 0

- name: Use Node.js 14.x
uses: actions/setup-node@v2
with:
node-version: 14.x

- name: Install Dependencies
run: yarn
- uses: ./.github/actions/ci-setup
- uses: ./.github/actions/ci-checks

- name: Create Release Pull Request or Publish to npm
uses: changesets/action@v1
Expand Down
2 changes: 0 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member Author

Choose a reason for hiding this comment

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

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

"changeset": "changeset",
"prerelease": "npm run build && npm run test",
Copy link
Member Author

Choose a reason for hiding this comment

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

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

"release": "changeset publish",
"version": "changeset version && node ./scripts/bump-peer-dep-ranges.js"
},
Expand Down
32 changes: 1 addition & 31 deletions packages/core/rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

const isSymbolObservable = t.buildMatchMemberExpression('Symbol.observable');
return {
visitor: {
MemberExpression(path) {
if (!isSymbolObservable(path.node)) {
return;
}
// class Interpreter { [Symbol.observable]() {} }
if (path.parentPath.isClassMethod()) {
path.parentPath.remove();
return;
}
// Interpreter.prototype[Symbol.observable] = function() {}
if (
path.parentPath.isMemberExpression() &&
path.parentPath.get('property') === path &&
path.parentPath.parentPath.isAssignmentExpression()
) {
path.parentPath.parentPath.remove();
return;
}
}
}
};
};

const createTsPlugin = ({ declaration = true, target } = {}) =>
typescript({
clean: true,
Expand All @@ -49,10 +22,7 @@ const createBabelPlugin = () =>
skipPreflightCheck: true,
babelHelpers: 'inline',
extensions: ['.ts', '.tsx', '.js'],
plugins: [
'babel-plugin-annotate-pure-calls',
stripSymbolObservableMethodPlugin
]
plugins: ['babel-plugin-annotate-pure-calls']
});

const createNpmConfig = ({ input, output }) => ({
Expand Down
20 changes: 16 additions & 4 deletions packages/core/src/Actor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ import {
ActorRef,
BaseActorRef
} from './types';
import { interopSymbols, isMachine, mapContext, toInvokeSource } from './utils';
import {
symbolObservableRef,
isMachine,
mapContext,
toInvokeSource
} from './utils';
import * as serviceScope from './serviceScope';

export interface Actor<
Expand Down Expand Up @@ -38,7 +43,9 @@ export function createNullActor(id: string): ActorRef<any> {
toJSON: () => ({
id
}),
...interopSymbols
[symbolObservableRef.symbol]: function () {
return this;
}
};
}

Expand Down Expand Up @@ -107,16 +114,21 @@ export function isSpawnedActor(item: any): item is ActorRef<any> {
return isActor(item) && 'id' in item;
}

// TODO: refactor the return type, this could be written in a better way but it's best to avoid unneccessary breaking changes now
export function toActorRef<
TEvent extends EventObject,
TEmitted = any,
TActorRefLike extends BaseActorRef<TEvent> = BaseActorRef<TEvent>
>(actorRefLike: TActorRefLike): ActorRef<TEvent, TEmitted> {
>(
actorRefLike: TActorRefLike
): ActorRef<TEvent, TEmitted> & Omit<TActorRefLike, keyof ActorRef<any, any>> {
return {
subscribe: () => ({ unsubscribe: () => void 0 }),
id: 'anonymous',
getSnapshot: () => undefined,
...interopSymbols,
[symbolObservableRef.symbol]: function () {
return this;
},
...actorRefLike
};
}
3 changes: 2 additions & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { mapState } from './mapState';
import { StateNode } from './StateNode';
import { State } from './State';
import { Machine, createMachine } from './Machine';
import { Actor } from './Actor';
import { Actor, toActorRef } from './Actor';
import * as actions from './actions';
import {
interpret,
Expand All @@ -18,6 +18,7 @@ const { assign, send, sendParent, sendUpdate, forwardTo, doneInvoke } = actions;

export {
Actor,
toActorRef,
Machine,
StateNode,
State,
Expand Down
47 changes: 24 additions & 23 deletions packages/core/src/interpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ import {
StopActionObject,
Subscription,
AnyState,
StateConfig
StateConfig,
InteropSubscribable
} from './types';
import { State, bindActionToState, isStateConfig } from './State';
import * as actionTypes from './actionTypes';
Expand All @@ -53,12 +54,11 @@ import {
toEventObject,
toSCXMLEvent,
reportUnhandledExceptionOnInvocation,
symbolObservable,
toInvokeSource,
toObserver,
isActor,
isBehavior,
interopSymbols
symbolObservableRef
} from './utils';
import { Scheduler } from './scheduler';
import { Actor, isSpawnedActor, createDeferredActor } from './Actor';
Expand Down Expand Up @@ -388,18 +388,18 @@ export class Interpreter<

return this;
}
public subscribe(
observer: Observer<
State<TContext, TEvent, any, TTypestate, TResolvedTypesMeta>
>
): Subscription;
public subscribe(
nextListener?: (
state: State<TContext, TEvent, any, TTypestate, TResolvedTypesMeta>
) => void,
errorListener?: (error: any) => void,
completeListener?: () => void
): Subscription;
public subscribe(
observer: Observer<
State<TContext, TEvent, any, TTypestate, TResolvedTypesMeta>
>
): Subscription;
public subscribe(
nextListenerOrObserver?:
| ((
Expand Down Expand Up @@ -1166,7 +1166,9 @@ export class Interpreter<
return { id };
},
getSnapshot: () => resolvedData,
...interopSymbols
[symbolObservableRef.symbol]: function () {
return this;
}
};

this.children.set(id, actor);
Expand Down Expand Up @@ -1208,11 +1210,12 @@ export class Interpreter<
id,
send: (event) => receivers.forEach((receiver) => receiver(event)),
subscribe: (next) => {
listeners.add(next);
const observer = toObserver(next);
listeners.add(observer.next);
Comment on lines +1213 to +1214
Copy link
Member Author

Choose a reason for hiding this comment

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

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


return {
unsubscribe: () => {
listeners.delete(next);
listeners.delete(observer.next);
}
};
},
Expand All @@ -1226,7 +1229,9 @@ export class Interpreter<
return { id };
},
getSnapshot: () => emitted,
...interopSymbols
[symbolObservableRef.symbol]: function () {
return this;
}
};

this.children.set(id, actor);
Expand Down Expand Up @@ -1265,7 +1270,9 @@ export class Interpreter<
toJSON() {
return { id };
},
...interopSymbols
[symbolObservableRef.symbol]: function () {
return this;
}
};

this.children.set(id, actor);
Expand Down Expand Up @@ -1310,7 +1317,9 @@ export class Interpreter<
toJSON() {
return { id };
},
...interopSymbols
[symbolObservableRef.symbol]: function () {
return this;
}
});
}

Expand Down Expand Up @@ -1357,15 +1366,7 @@ export class Interpreter<
};
}

public [symbolObservable](): Subscribable<
State<TContext, TEvent, TStateSchema, TTypestate, TResolvedTypesMeta>
> {
return this;
}

// this gets stripped by Babel to avoid having "undefined" property in environments without this non-standard Symbol
// it has to be here to be included in the generated .d.ts
public [Symbol.observable](): Subscribable<
public [symbolObservableRef.symbol](): InteropSubscribable<
State<TContext, TEvent, TStateSchema, TTypestate, TResolvedTypesMeta>
> {
return this;
Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1603,16 +1603,20 @@ export interface Subscription {
}

export interface InteropObservable<T> {
[Symbol.observable]: () => Subscribable<T>;
[Symbol.observable]: () => InteropSubscribable<T>;
}

export interface Subscribable<T> {
export interface InteropSubscribable<T> {
subscribe(observer: Observer<T>): Subscription;
}
Comment on lines +1609 to +1611
Copy link
Member Author

Choose a reason for hiding this comment

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

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:

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.


export interface Subscribable<T> extends InteropSubscribable<T> {
subscribe(observer: Observer<T>): Subscription;
subscribe(
next: (value: T) => void,
error?: (error: any) => void,
complete?: () => void
): Subscription;
subscribe(observer: Observer<T>): Subscription;
}

export type Spawnable =
Expand Down
11 changes: 7 additions & 4 deletions packages/core/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -529,15 +529,18 @@ export function isObservable<T>(value: any): value is Subscribable<T> {
}
}

export const symbolObservable = (() =>
const symbolObservable = (() =>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

(typeof Symbol === 'function' && Symbol.observable) || '@@observable')();

export const symbolObservableRef = {
symbol: symbolObservable as Exclude<typeof symbolObservable, string>
} as const;

// TODO: to be removed in v5, left it out just to minimize the scope of the change and maintain compatibility with older versions of integration paackages
export const interopSymbols = {
[symbolObservable]: function () {
[symbolObservableRef.symbol]: function () {
return this;
},
// this gets stripped by Babel to avoid having "undefined" property in environments without this non-standard Symbol
// it has to be here to be included in the generated .d.ts
[Symbol.observable]: function () {
return this;
}
Expand Down
Loading