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

Improve React usePage generic type #1451

Merged
merged 2 commits into from
Feb 17, 2023
Merged

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Feb 15, 2023

In #1396, the usePage generic type was (re)introduced for the React adapter.

When trying to use this, I've found it to be quite inconvenient because it's expecting us to pass in an entire Page type, which has things like the url, component, etc. with the props being nested under a props key.

This PR aims to solve that by changing the generic type to just accept the PageProps type.

This would also make the usage consistent with the Vue version.

@jessarcher
Copy link
Member Author

@Bloemendaal, @hailwood - I'd appreciate your thoughts on this one 🙂

@Bloemendaal
Copy link
Contributor

Looks like an improvement to me! 👍

Just wondering if there is any way (or reason) to extend the Page interface itself, since this is a property in the createInertiaApp object...

@jessarcher
Copy link
Member Author

jessarcher commented Feb 17, 2023

Just wondering if there is any way (or reason) to extend the Page interface itself, since this is a property in the createInertiaApp object...

I can't think of a reason you'd want to, but perhaps my imagination is lacking 😆

The Page interface is currently only exported from the @inertiajs/core package and we discourage importing anything directly from that package as it should not be a direct dependency of your application.

If someone had a good reason I don't think I'd have any problem re-exporting it from the adapters.

@jessarcher jessarcher merged commit 6ceb054 into master Feb 17, 2023
@jessarcher jessarcher deleted the improve-react-usepage-generic branch February 17, 2023 00:17
@hailwood
Copy link
Contributor

Yep that's exactly how I'd expect to use it 👍
Very nice 🙂

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.

3 participants