refactor: Move fetchNearestIntersection to the state machine

[joshlarson]

Asana Ticket: https://app.asana.com/0/1148853526253420/1207539825544288/f

[joshlarson]

Discussion about this:

This change will cause fetch-nearest-intersection to be invoked any time a waypoint is placed. This is a change from the hook implementation, where it was only called when the start point was placed (or changed).

I don't think it's a huge problem for right now, which is why I'm PR'ing this as-is, but I do wonder if the Right Solution is to add another state, so that the drawing-detour states would be something like:

• No Start Point
• Start Point Only <-- calls fetchNearestIntersection and eventually fetchUnfinishedDetour
• Some Waypoints <-- calls fetchDetourDirections
• End Point Placed <-- calls fetchFinishedDetour

@mbta/skate-developers thoughts?

[joshlarson]

Hmmm, this also seems to leave open a race condition, where if the end point is placed before the fetch-nearest-intersection actor finished its work, then the fetch-nearest-intersection actor's work is ignored, and we end up with From null. Is there a way to force an actor to keep plugging on even if the state machine has moved to a different state?

[firestack]

You want to spawn an actor
A while ago, I tried doing this refactor once and ran into this, and was struggling to grasp the concepts and how to use spawn for this, maybe you'll have better luck, or we can try pairing on it.

[firestack]

If we want to avoid spawning, we'll probably need to do the intermediate state thing,
which could be done within the place start point state and use onDone to transition out of place start point and into place waypoint

Spawning has the benefit of not blocking the user on the intersection fetch, but then means we have to figure out how to store it and figure out how that gets serialized and persisted, so that we're not storing an actor state we don't mean to.

[joshlarson]

Okay here's a silly question - because this tool is primarily (exclusively?) used by dispatchers, where the internet connection is reasonably reliable, can we roll forward with this as-is, and migrate towards spawn later?

I think spawn is the right call in the long/medium term, but the spawn contract is so different from invoke that actually making that change is more complicated than I was expecting.

I think though that merging this as-is should unblock the things that are blocked, and changing to spawn shouldn't change anything (aside from fixing the issues mentioned ☝️☝️)

[firestack]

I think that's fine, would you document your reasoning inline? that might help future "us"?

My day is up, but I'll revisit this first thing tomorrow

[joshlarson]

Okay! I'll be around a little bit tomorrow, so let's see how we're feeling about this tomorrow.

[joshlarson]

This is why I noticed this, because if this await act is moved down below the second fireEvent.click, then the test fails.

[github-actions]

Coverage of commit 60655eb

Summary coverage rate:
  lines......: 93.0% (3288 of 3534 lines)
  functions..: 72.5% (1357 of 1871 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report