Skip to content

fix: add missing state property to interface#1579

Merged
marvinhagemeister merged 1 commit into
freshframework:mainfrom
csvn:404-props-state
Aug 8, 2023
Merged

fix: add missing state property to interface#1579
marvinhagemeister merged 1 commit into
freshframework:mainfrom
csvn:404-props-state

Conversation

@csvn

@csvn csvn commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

Corrects the props interface used in 404 pages to reflect that state is available as well.

Closes #1578

@deer

deer commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

Thanks for taking the initiative to open a PR. What about adding a test to make sure this doesn't get broken in the future?

@csvn

csvn commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

@deer I could not find any references on how to test pure Typescript changes/types. There's no existing types_test.ts to add to. Would this mean creating a new fixture_??? purely to check types? Should we create a types_test.ts that is just type checked and does something like below?

export default function NotFound(props: UnknownPageProps<{ foo: number }, { bar: boolean }>) {
  props.data.foo satisfies number;
  props.state.bar satisfies boolean;
}

The above would be enough to test the UnknownPageProps type, but not sure where to put it to be type checked.

@deer

deer commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

I meant a functional test that confirms the state is actually passed into the test _404 page. Currently there's a tests/fixture/routes/_404.tsx file that you could possibly modify. Look at how the tests are done in the main_test.ts file. Perhaps you can piggyback on an existing _middleware.ts which sets the state, but try going to an invalid url. That should get the state set, and then route to the 404.

@csvn

csvn commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

I gave it a try. I added some types for the generics, and updated the _404.tsx file.
Not sure if I should add a new test case, or updating existing one was enough

Let me know what you think 😄

@deer deer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this testing approach is fine.

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for making a PR 👍

@marvinhagemeister

Copy link
Copy Markdown
Contributor

Looks like the CI is failing due to formatting issues. You can resolve these by running deno fmt locally and committing the changes.

Corrects the props interface used in 404 pages to reflect that
`state` is available as well.

Closes #1578
@csvn

csvn commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

Sorry, thought I had format on automatically on save, and workflow did not when I made the PR. Hope fully it should be OK now 😀

@csvn csvn requested a review from marvinhagemeister August 4, 2023 12:07
@marvinhagemeister marvinhagemeister merged commit 7d4c751 into freshframework:main Aug 8, 2023
@csvn csvn deleted the 404-props-state branch August 8, 2023 12:59
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.

404 page props are missing state

3 participants