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

fix(gatsby): Use xstate predictableActionArguments & update to 4.34 #36342

Merged
merged 13 commits into from
Nov 17, 2022

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Aug 9, 2022

Description

Since statelyai/xstate#3289 there's a warning printed to the console now to use this option. Trying this out if tests break 🤷

Also works around removal of meta.state property mentioned in:

Please be aware that you might not able to use state from the meta argument when using this flag.

Related Issues

[ch54148]

@LekoArts LekoArts added the topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) label Aug 9, 2022
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 9, 2022
@LekoArts LekoArts changed the title fix(gatsby): Use predictableActionArguments fix(gatsby): Use xstate predictableActionArguments & update to 4.33 Aug 9, 2022
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 9, 2022
@pieh
Copy link
Contributor

pieh commented Aug 16, 2022

@Andarist maybe you can help us out on another one :)

calculatingDirtyQueries: {
entry: assign<IQueryRunningContext>(({ pendingQueryRuns }) => {
return {
pendingQueryRuns: new Set(),
currentlyHandledPendingQueryRuns: pendingQueryRuns,
}
}),
invoke: {
id: `calculating-dirty-queries`,
src: `calculateDirtyQueries`,
onDone: {
target: `runningStaticQueries`,
actions: [
`assignDirtyQueries`,
`clearCurrentlyHandledPendingQueryRuns`,
],
},
},
},

When predictableActionArguments: true is used the order of things changes on this state.

Before it would first do entry / assign and then invoke calculateDirtyQueries service, but with changes in this PR, it first invoke calculateDirtyQueries service and then does entry / assign - is this expected?

The setup for this is done to grab current batch list (pendingQueryRuns) and pass it to service, while reseting it globally so if new entry would show up while we are in service it wouldn't get lost (just would need to wait for another for its turn / next batch execution) - is there better setup to achieve something like that?

@Andarist
Copy link
Contributor

This is essentially a byproduct of how those are concatenated here. I could reverse this but then it wouldn't be possible to do this:

entry: send({ type: 'PING' }, { to: 'child' }),
invoke: {
  id: 'child',
  src: childMachine
}

We could introduce more custom logic for this case to fix this though as I think it makes sense in this case for invoke to have access to the updated context.

I need to think about the potential implementation in v4 as this is a little bit tricky - it doesn't help that the upcoming v5 has much better architecture internally, with which this wouldn't be a problem at all 😬

@pieh
Copy link
Contributor

pieh commented Aug 17, 2022

We could introduce more custom logic for this case to fix this though as I think it makes sense in this case for invoke to have access to the updated context.

I need to think about the potential implementation in v4 as this is a little bit tricky - it doesn't help that the upcoming v5 has much better architecture internally, with which this wouldn't be a problem at all 😬

Please, do let us know wether we should expect changes in this behavior. Maybe consider temporarily disabling the warning that is printed to console about predectableActionArguments until there is clarity on what the behavior of that mode should be? I do think in meantime we will pin xstate to previous minor, as our users are seeing those warnings which we are trying to "silence" with this PR

@pieh pieh mentioned this pull request Aug 17, 2022
@Andarist
Copy link
Contributor

Please, do let us know wether we should expect changes in this behavior. Maybe consider temporarily disabling the warning that is printed to console about predectableActionArguments until there is clarity on what the behavior of that mode should be?

I think that I've figured out a potential way forward yesterday. I need to give a try implementing it but I'm hopeful - what you have discovered here was not an intentional change, so I very much would like to fix this for you and other users.

I do think in meantime we will pin xstate to previous minor, as our users are seeing those warnings which we are trying to "silence" with this PR

That's a good move for you right now - I won't be able to push a fix today and I don't want to turn off the warning just to enable it again in a couple of days.

@LekoArts LekoArts changed the title fix(gatsby): Use xstate predictableActionArguments & update to 4.33 fix(gatsby): Use xstate predictableActionArguments & update to 4.34 Nov 16, 2022
@LekoArts
Copy link
Contributor Author

@Andarist heh, it's been a while... I just merged your PRs and update to latest xstate, let' see if tests are happy

@LekoArts LekoArts marked this pull request as ready for review November 16, 2022 10:48
@Andarist
Copy link
Contributor

Let me know if you'd like any further assistance with this.

@LekoArts
Copy link
Contributor Author

I initially merged #36393 but that failed some tests and for now I'm fine with it being not optimal but working :D

Appreciate your input and help!

mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
…gatsbyjs#36342)

* update version and fix types

* yolo add option

* Work around removal of meta.state

* Refactor `meta.state` access in `gatsby-source-filesystem` (gatsbyjs#36355)

Co-authored-by: Lennart <lekoarts@gmail.com>

* fix ts-ignore

* Upgrade XState to the latest 4.33.3 (gatsbyjs#36451)

* update to latest xstate

* Use `pendingQueryRuns` available on the event (gatsbyjs#36393)

* Revert "Use `pendingQueryRuns` available on the event (gatsbyjs#36393)"

This reverts commit 9460dd3.

Co-authored-by: tyhopp <hopp.ty.c@gmail.com>
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Co-authored-by: Ty Hopp <tyhopp@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants