-
Notifications
You must be signed in to change notification settings - Fork 349
feat: start of cross platform page system #4731
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
Conversation
| ...generatedState, | ||
| } | ||
|
|
||
| const [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to api-client, see notion, we need to figure out how we're going to do this (the full thing, not just tags) for the app-frontend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this no longer catch errors and display the errors generating state banner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors are stored still;
const errors: unknown[] = []
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const handleError = (err: any, defaultValue: any) => {
console.error('Error fetching state data:', err)
errors.push(err)
return defaultValue
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Inside the impl)
|
|
||
| export interface GeneratedState extends Labrinth.State.GeneratedState { | ||
| // Additional runtime-defined fields not from the API | ||
| projectTypes: ProjectType[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see other comment, how are we going to handle this on app-frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a cross-platform page system starting with the servers pages. It sets up TanStack Vue Query as a cross-platform alternative to useAsyncData, migrates the server list page to the new @modrinth/ui package, and moves API client dependency injection to @modrinth/ui.
Key changes:
- Added
@tanstack/vue-queryas a cross-platform data fetching solution - Migrated servers page components to
@modrinth/uipackage for reuse across app-frontend and frontend - Restructured API client types into namespaced modules (Labrinth, Archon, Kyros, ISO3166)
- Moved API client provider from frontend app to shared UI package
Reviewed Changes
Copilot reviewed 77 out of 80 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Added @tanstack/vue-query, papaparse, and related dependencies |
packages/ui/src/pages/servers/manage/index.vue |
New cross-platform server list page using TanStack Query instead of useAsyncData |
packages/ui/src/providers/api-client.ts |
New provider for Modrinth API client DI context |
packages/ui/src/utils/product-utils.ts |
New utility functions for product/billing data formatting |
packages/api-client/src/modules/labrinth/types.ts |
Consolidated API types into namespaced Labrinth namespace |
packages/api-client/src/modules/archon/* |
New Archon API modules for server management endpoints |
packages/api-client/src/modules/kyros/* |
New Kyros API modules for filesystem operations |
packages/api-client/src/modules/iso3166/* |
New ISO3166 module for country/subdivision data |
packages/api-client/src/modules/labrinth/billing/internal.ts |
New billing API module with subscription and payment methods |
apps/frontend/src/pages/servers/manage/index.vue |
Simplified to use cross-platform component from @modrinth/ui |
apps/frontend/src/plugins/tanstack.ts |
New Nuxt plugin for TanStack Query setup with SSR support |
apps/app-frontend/src/routes.js |
Added servers route using shared component |
apps/app-frontend/src/main.js |
Registered VueQueryPlugin |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0a5de4a to
57265da
Compare
Currently this is only going to be used by the servers pages, but I dont see why we cant use this for orgs, users, collections etc.? Out of scope though
@tanstack/vue-querywhich is a cross platformuseAsyncDataalternative - this will only be used in the@modrinth/uipackage and potentially the app frontend in the future?Set up a system inside of@modrinth/uifor automatic page registration on nuxt + app frontend@modrinth/ui.Places to look:
packages/ui/src/pages- I tried to make a dynamic page system, however i was running into issues with nuxt, so I think it's probably just easier to just create stubs on the frontend, this isn't great... For the app-frontend we can just pass the page from@modrinth/uiinto the route object in the routes.js, which is good.packages/ui/src/pages/servers/manage/index.vue- Take a look at tanstack query, it's a cross platform useAsyncData alternative (that's actually better in most cases) - read more: https://tanstack.com/query/latest/docs/framework/vue/overviewpackages/ui/src/providers/api-client.ts- abstracted