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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(gatsby-source-filesystem): Upgrade XState and refactor its usage #17192

Merged

Conversation

@Andarist
Copy link
Contributor

commented Aug 29, 2019

Description

I was exploring the codebase to see how you are using XState and I've encountered this old machine using barebones Machine API from older version (v3) of XState. So I've decided to refactor this to use Interpreter (which you are also using in other parts of the codebase), mainly as an exercise for myself but I believe that it's cleaner now and gives better visualization than the previous version:
Screenshot 2019-09-03 at 13 24 39

I strongly believe this should behave exactly the same as the previous implementation, BUT I have no idea how to test this properly. I could use a hint regarding that from you 馃槈

I've also left one TODO comment which should be resolved before merging (even if resolving just means removing it - I was unsure what's the proper logic for this one, I've decided to keep it like it was for now). In addition to that I've noticed that processed nodes before bootstrap is don are not reported - shouldn't maybe those logs be queued and flushed later?

@Andarist Andarist requested a review from gatsbyjs/core as a code owner Aug 29, 2019
@Andarist Andarist force-pushed the Andarist:gatsby-source-filesystem/xstate-upgrade branch from a8b6eac to 4e78531 Aug 30, 2019
@Andarist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

I get this on CI:

'gatsby-source-filesystem' doesn't seem to be installed. Restart gatsby-dev to publish it

But ain't sure what I should do to fix it, or even try this locally.

@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

In addition to that I've noticed that processed nodes before bootstrap is don are not reported - shouldn't maybe those logs be queued and flushed later?

Reason for this is mostly that initial node creation (during bootstrap) may result in thousands of File nodes, so this would be very spammy. Node creation after bootstrap is finished is result of file changes and for most use cases this will be single new file / file changes at a time (editing files locally)

I'll take a look on this (why it fails in CI) - I agree this is a lot cleaner than current implementation

I get this on CI:

'gatsby-source-filesystem' doesn't seem to be installed. Restart gatsby-dev to publish it

But ain't sure what I should do to fix it, or even try this locally.

This is quirk of our hacky utility for testing changes in packages (gatsby-dev) and we didn't prfioritize fixing this false warning. This can be ignored

@pieh pieh self-assigned this Sep 3, 2019
@pieh pieh changed the title Upgrade XState and refactor its usage in gatsby-source-filesystem refactor(gatsby-source-filesystem): Upgrade XState and refactor its usage Sep 3, 2019
Copy link
Contributor

left a comment

This is awesome! I left few notes, but foundation is very solid

packages/gatsby-source-filesystem/src/gatsby-node.js Outdated Show resolved Hide resolved
},
},
},
// TODO: those two were not restricted to READY state, but maybe they should? or even maybe it should queue in NOT_READY?

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

This is very nice illustration of why using actions in state machine definition is so great (opposed to what we currently have) - it made it very clear that this is not handled properly. Those definitely need to have a bit different handling in CHOKIDAR.NOT_READY and CHOKIDAR.READY

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

That said, this might be not in scope of this PR (it preserves current handling) - so I'll leave this up to you if you want to attack this. If you don't - that's ok, let's just leave this TODO here

This comment has been minimized.

Copy link
@Andarist

Andarist Sep 3, 2019

Author Contributor

I would prefer keeping this as is now. I can try to fix this in a followup PR though - if only you could give me hints on how the handling should differ. Should those be queued until ready?

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

Yup, let's try to minimize scope of the PR. This will allow faster iteration.

},
// TODO: those two were not restricted to READY state, but maybe they should? or even maybe it should queue in NOT_READY?
on: {
CHOKIDAR_CHANGE: { actions: `createAndProcessNode` },

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

This can be handled in exactly same way as CHOKIDAR_ADD event (so queue in NOT_READY and process immediately in READY)

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

Maybe even CHOKIDAR_ADD / CHOKIDAR_CHANGE event can be merged because as far as plugin is concerned there's not much difference between creating and updating nodes (gatsby core takes care of this stuff).

But I don't think that verbosity of events is any problem and this might be easier to read and understand

This comment has been minimized.

Copy link
@Andarist

Andarist Sep 3, 2019

Author Contributor

Ok, so you have actually answered here my previous question 馃槄 Still would be in favor of keeping current logic for now, I can prepare a PR with that queuing immediately after this would get merged in.

As to verbosity of events - I would be in favor of keeping separate names.

// TODO: those two were not restricted to READY state, but maybe they should? or even maybe it should queue in NOT_READY?
on: {
CHOKIDAR_CHANGE: { actions: `createAndProcessNode` },
CHOKIDAR_UNLINK: { actions: `deleteNode` },

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

I do think this need similar handling as CHOKIDAR_ADD / CHOKIDAR_CHANGE:

We should queue it when it's not ready. I think we do need single queue for additions, changes and deletions - so potential queue could look like this:

[
  {
    type: 'add',
    path: '<some_path>'
  },
  {
    type: 'del',
    path: '<some_path>'
  }
]

so then flushing queue would handle those in proper order

deleteNode({ node })
}
},
flushPathQueue(_, { resolve, reject }) {

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

I spent embarrassingly long time looking where resolve / reject is coming from (initially I thought this is how xstate handles async action, before I found that resolve/reject is send in fsMachine.send({ type: `CHOKIDAR_READY`, resolve, reject })

I do think we can rework it to be more inline with idiomatic xstate by:

  • creating new state in CHOKIDAR (sibling to CHOKIDAR.NOT_READY) - let's call it FLUSHING
		  states: {
            NOT_READY: {
              on: {
-               CHOKIDAR_READY: `READY`,
+               CHOKIDAR_READY: `FLUSHING`,
                CHOKIDAR_ADD: { actions: `queueNodeProcessing` },
              },
            },
+           FLUSHING: {
+             invoke: {
+               src: () => {
+                 return flushPathQueue()
+               },
+               onDone: {
+                 target: `READY`,
+               },
+             },
+           },
            READY: {
              on: {
                CHOKIDAR_ADD: { actions: `createAndProcessNode` },
              },
            },
          },

see https://xstate.js.org/docs/guides/communication.html#invoking-services for docs about invoke property

  • change the promise returned from sourceNodes a bit, let chokidar.on(`ready`) just send event to machine, and let's wait for machine to reach CHOKIDAR.READY to resolve that promise

Making it this way also shows that there is another potential problem (it's good thing) - what happens currently when chokidar emits event while flushing is in progress? Right now we would not queue work, but instead would process it immediately - making order of processing potentially incorrect. So I think while we are flushing, we should queue chokidar events and when flushing finishes check if queue is empty (then we can go to CHOKIDAR.READY state, and if it's not - flush again and repeat that until queue is empty)

This comment has been minimized.

Copy link
@Andarist

Andarist Sep 3, 2019

Author Contributor

I like that 馃憤

One question though - flushPathQueue might reject and this would stay unhandled. I have no idea under what circumstances it could reject though? So not sure how this should be handled and for what to await for in ready listener to reject returned promise there.

This comment has been minimized.

Copy link
@Andarist

Andarist Sep 3, 2019

Author Contributor

And again - maybe we could pursue fixing this in a followup PR? Or do you prefer keeping it as part of this one?

This comment has been minimized.

Copy link
@pieh

pieh Sep 3, 2019

Contributor

Right, my idea here is certainly not fully fleshed out (just starting point).

And also agree let's not scope creep this PR - let's make this PR 1 to 1 refactor (keeping current behaviour with all its flaws). All those edge cases that were uncovered can be incrementally handled in future pull requests

This comment has been minimized.

Copy link
@Andarist

Andarist Sep 3, 2019

Author Contributor

Cool, I believe it's 1 to 1 right now if I haven't made any stupid mistake. I still don't know how to test this properly though - so I would appreciate you taking a look at this or giving me guidance.

@Andarist

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@pieh thanks for the initial review!

@Andarist Andarist force-pushed the Andarist:gatsby-source-filesystem/xstate-upgrade branch from 4e78531 to d67e1b4 Sep 3, 2019
pieh and others added 2 commits Sep 3, 2019
@pieh

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

I added basic test suite (that tests sourceNodes rather than machine itself) with couple of tests skipped (edge cases discovered in this review).

I'll merge this in

@pieh
pieh approved these changes Sep 4, 2019
Copy link
Contributor

left a comment

Thank for cleaning this up! It's much easier to follow now!

@pieh pieh merged commit 9185ece into gatsbyjs:master Sep 4, 2019
20 checks passed
20 checks passed
Danger All good
Details
Peril All green. Well done.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: themes_e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
ci/circleci: windows_unit_tests Your tests passed on CircleCI!
Details
cypress: default-group 67 tests passed in 00:30
Details
unit_tests_windows Build #20190904.60 succeeded
Details
@gatsbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Holy buckets, @Andarist 鈥 we just merged your PR to Gatsby! 馃挭馃挏

Gatsby is built by awesome people like you. Let us say 鈥渢hanks鈥 in two ways:

  1. We鈥檇 like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We鈥檝e got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions 鈥斅爓ink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you鈥檒l be able to label issues, review pull requests, and merge approved pull requests.

If there鈥檚 anything we can do to help, please don鈥檛 hesitate to reach out to us: tweet at @gatsbyjs and we鈥檒l come a-runnin鈥.

Thanks again!

@Andarist

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@pieh cool, I鈥檒l work on fixing those issues discovered during the PR in following days

@Andarist Andarist deleted the Andarist:gatsby-source-filesystem/xstate-upgrade branch Sep 4, 2019
waltercruz added a commit to waltercruz/gatsby that referenced this pull request Sep 8, 2019
* Upgrade XState and refactor its usage in gatsby-source-filesystem

* test: add basic test suite for sourceNodes
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Thanks for all the great work here @Andarist

Featuring this in the September Gazette in #17548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.