-
Notifications
You must be signed in to change notification settings - Fork 116
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
Swap usePlatforms for usePresets/usePreset #1139
Conversation
@@ -288,6 +277,24 @@ export function useLibraries(): LibraryVersions[] { | |||
return data.libraries | |||
} | |||
|
|||
export function usePresets(platform: string | undefined): Preset[] { | |||
const getByPlatform = ([url, platform]) => { | |||
return get(url && platform && `${url}?platform=${platform}&page_size=100`) |
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.
I just realized the site will cease to show any more than 100 presets with this change. I think what we eventually want is a searchable thing with auto-populated results, and then an "Add new..." for creating a new preset
For now, instead of usePresets, we might want to keep what we had before, so we're sure not to omit any presets from the dropdown
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.
It's 100 presets per platform, which I think will be fine until we get around to revamping it (i.e. when we let users create presets)
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.
We could also bump up 100 to 250 here
decomp.me/backend/coreapp/views/preset.py
Line 20 in 66c892f
max_page_size = 100 |
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.
oh, right... yeah it's probably fine then
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.
Can we add an assert for this on the backend perhaps? Or implement pagination here
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.
Assert that the length of presets is < 100? Currently we have 43 presets for N64 (which I'm assuming is the most popular platform). I'll create an issue to make the Presets a searchable drop-down - I feel it's likely we'll resolve that before we hit 100 presets for any individual platform.
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.
Created #1152.
With HTTP/2 doing lots of requests at once shouldn't be too bad. Particularly if we start caching the frontend / doing async django in future. |
The upside to this change is that rather than getting all compilers + platforms + presets etc via the
/compiler
, we grab each preset as required.The downside to this change is that rather than getting all compilers + platforms + presets etc via the
/compiler
, we grab each preset as required.I still think this is much better in terms of amount of data being sent & processed - but it is more requests to the backend (I know there is a maximum of 6 outstanding requests per tab, does this also apply to
fetch
)?Gonna see how the vercel deployment looks...Edit:
Performance seems good to me... When loading the front page we load ~20 scratches, worst-case scenario would be 20 different presets, but currently there are only 5 requests needed (due to scratches without any preset, as well as a number of scratches using the same preset). Loading the platform page for n64 only requires 3 calls to the presets endpoint, so I think it's OK.
Bugs:
Scratch.preset == '97' but no preset with that id was found.
🟢 Fixed!