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

Deprecate send() action creator #3393

Merged
merged 20 commits into from
Feb 12, 2023
Merged

Deprecate send() action creator #3393

merged 20 commits into from
Feb 12, 2023

Conversation

davidkpiano
Copy link
Member

This PR deprecates the send() action creator in favor of sendTo() and raise().

@davidkpiano davidkpiano requested a review from Andarist June 8, 2022 14:31
@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2022

🦋 Changeset detected

Latest commit: 2637bab

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

This PR includes changesets to release 1 package
Name Type
xstate Minor

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

@ghost
Copy link

ghost commented Jun 8, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 8, 2022

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.

@davidkpiano
Copy link
Member Author

@Andarist Is this good to go?

Copy link
Contributor

@mellson mellson left a comment

Choose a reason for hiding this comment

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

It looks good to me, based on what little knowledge I have of this codebase so far 😊

): RaiseAction<TEvent> | SendAction<TContext, AnyEventObject, TEvent> {
if (!isString(event)) {
return send(event, { to: SpecialTargets.Internal });
event: Event<TEvent>,
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't support expressions right now. I think that we should bring it to here - otherwise not all sends can be refactored to raise

Comment on lines 163 to 166
): RaiseAction<TEvent> {
if (!isString(event) || options) {
return send(event, { ...options, to: SpecialTargets.Internal }) as any;
}
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should stop piggybacking on send here. It's especially important right now because you are suppressing the type checker to satisfy the declared return type. I wouldn't be surprised if we miss something that should be handled because of that, now or in the future. At times, we are checking action.type after all.

@@ -253,23 +253,23 @@ describe('Raise events', () => {
{
actions: raise({
type: 'ALOHA'
}) as any /* TODO: FIX */,
}),
Copy link
Member

Choose a reason for hiding this comment

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

I created a spike for type-related improvements around this here: #3694

service.send('TO_B');
});

it('should be able to send an event to itself', (done) => {
Copy link
Member

Choose a reason for hiding this comment

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

This test case has the very same title as the one above. This should be fixed as I'm not exactly sure what this test is testing here.

Comment on lines 2663 to 2668
const service = interpret(machine).start();

service.onDone(() => done());

// Ensures that the delayed self-event is sent when in the `b` state
service.send('TO_B');
Copy link
Member

Choose a reason for hiding this comment

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

This test isn't testing anything right now - it has 0 expectations ;p

@Andarist Andarist force-pushed the davidkpiano/deprecate-send branch 3 times, most recently from 466d4b1 to b0bb50d Compare January 27, 2023 13:07
* Stop reusing `send` in `raise`

* Fixed `actionToSCXML` for `raise`

* More tests
# Conflicts:
#	packages/core/src/StateNode.ts
#	packages/core/src/actions.ts
#	packages/core/src/interpreter.ts
#	packages/core/src/types.ts
#	packages/core/test/interpreter.test.ts
#	packages/xstate-scxml/src/index.ts
# Conflicts:
#	packages/core/src/actions.ts
# Conflicts:
#	packages/xstate-react/src/useInterpret.ts
@Andarist Andarist merged commit 430986c into main Feb 12, 2023
@Andarist Andarist deleted the davidkpiano/deprecate-send branch February 12, 2023 07:57
@github-actions github-actions bot mentioned this pull request Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants