chore(gatsby): convert page-component to typescript

[Kornil]

Convert src/redux/machines/page-component.js and test file to TypeScript.

Related Issues

#21995

[pieh]

Lot of our machine is still untyped - we do have type for context, but we don't have types for state schema or possible events (using AnyEvent). Do you think you would be able to type it more (as shown in https://xstate.js.org/docs/guides/typescript.html )?

[Kornil]

Thank you for the feedback, I will address it all in the next few days!

[Kornil]

I addressed all feedback @pieh, I kept your comment unresolved but added a 👍 to indicate I solved the issue, if you feel it was addressed please feel free to resolve them.

there are 2 small changes to the actual code (not types) that I'd like to highlight since they have the potential to impact the code itself:

• on line 137 the if statement has an && instead of || I feel that was the intention by looking at the code (types would fail too with an ||
• line 165 I added an if to remove the page. If we can assume that page will always be in the payload, there is no difference. Otherwise, it will continue gracefully.

[pieh]

Thanks for the updates, will dive into them in a sec.

Do you think if it would be possible to change this types

type ActionTypes =
  | "BOOTSTRAP_FINISHED"
  | "DELETE_PAGE"
  | "NEW_PAGE_CREATED"
  | "PAGE_CONTEXT_MODIFIED"
  | "QUERY_EXTRACTION_GRAPHQL_ERROR"
  | "QUERY_EXTRACTION_BABEL_ERROR"
  | "QUERY_EXTRACTION_BABEL_SUCCESS"
  | "QUERY_CHANGED"
  | "QUERY_DID_NOT_CHANGE"
  | "QUERIES_COMPLETE"

export interface IEvent {
  type: ActionTypes
  path?: string
  query?: string
  page?: { path: string }
}

into something like this

type Actions =
  | { type: "BOOTSTRAP_FINISHED" }
  | { type: "QUERY_CHANGED", query: string }
  [...]

so we don't have those optional fields and instead had guarantees about fields?

I didn't try it, so I'm not sure if this would mean we have to cast events in action callbacks or it would magically work? (I don't see how this could work without casting tbh, given how almost everything is text based configuration but who knows)

[Kornil]

@pieh I tried that approach but unfortunately, it does require casting. I opted for these optional fields because it offers more room to play around with the payloads while highlighting the possible issues (like on line 165).

A possible refactor could be to make all payloads follow the same structure (they potentially could) but I fear that would be too outside the scope of this PR.

What is your opinion?

[pieh]

I suspected that's the case with union actions instead of union actions types :(

I think we will move forward with current approach, but it would be good to add some comments why we didn't go with Actions and went with ActionTypes union.

I will take a look into those changes you highlighted to make sure we are good

[Kornil]

@pieh I addressed all feedback and added a comment explaining the action types choice. Let me know if you have any suggestions to improve it.

I can't figure out why lint breaks during the commit stage, it is linting locally, just with the wrong config 😕

[pieh]

Don't worry about formatting part (other than being frustrated by it I guess) - I will run formatter before merging.

We do have formatting setup with git hooks - so committing also trigger eslint --fix (which also runs prettier) so maybe some conflict there? not sure tbh why it happens for you :(

[pieh]

I'll run format, so it's formatted using our config, but the code looks good now to me and I think we will be able to merge today (given that there are no issues in other tests/ci checks)

[pieh]

Also just to be sure - will sync master in

[pieh]

Looks great, let's get this in.

Thanks @Kornil for iterating on it!