-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
"changeset": "changeset", | ||
"prerelease": "npm run build && npm run test", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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, | ||
|
@@ -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 }) => ({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,8 @@ import { | |
StopActionObject, | ||
Subscription, | ||
AnyState, | ||
StateConfig | ||
StateConfig, | ||
InteropSubscribable | ||
} from './types'; | ||
import { State, bindActionToState, isStateConfig } from './State'; | ||
import * as actionTypes from './actionTypes'; | ||
|
@@ -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'; | ||
|
@@ -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?: | ||
| (( | ||
|
@@ -1166,7 +1166,9 @@ export class Interpreter< | |
return { id }; | ||
}, | ||
getSnapshot: () => resolvedData, | ||
...interopSymbols | ||
[symbolObservableRef.symbol]: function () { | ||
return this; | ||
} | ||
}; | ||
|
||
this.children.set(id, actor); | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a fix from the perspective of the |
||
|
||
return { | ||
unsubscribe: () => { | ||
listeners.delete(next); | ||
listeners.delete(observer.next); | ||
} | ||
}; | ||
}, | ||
|
@@ -1226,7 +1229,9 @@ export class Interpreter< | |
return { id }; | ||
}, | ||
getSnapshot: () => emitted, | ||
...interopSymbols | ||
[symbolObservableRef.symbol]: function () { | ||
return this; | ||
} | ||
}; | ||
|
||
this.children.set(id, actor); | ||
|
@@ -1265,7 +1270,9 @@ export class Interpreter< | |
toJSON() { | ||
return { id }; | ||
}, | ||
...interopSymbols | ||
[symbolObservableRef.symbol]: function () { | ||
return this; | ||
} | ||
}; | ||
|
||
this.children.set(id, actor); | ||
|
@@ -1310,7 +1317,9 @@ export class Interpreter< | |
toJSON() { | ||
return { id }; | ||
}, | ||
...interopSymbols | ||
[symbolObservableRef.symbol]: function () { | ||
return this; | ||
} | ||
}); | ||
} | ||
|
||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a way, this breaks the previous contract exposed under So, in fact, this interface only accepts observers - it doesn't accept separate listener functions for By implementing this change and using this as an explicit return type of the xstate/packages/core/src/interpreter.ts Lines 1369 to 1373 in d912a8d
Thanks to that we can revert the reordering of the 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 |
||||||||||||
|
||||||||||||
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 = | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -529,15 +529,18 @@ export function isObservable<T>(value: any): value is Subscribable<T> { | |
} | ||
} | ||
|
||
export const symbolObservable = (() => | ||
const symbolObservable = (() => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
There was a problem hiding this comment.
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