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

Route data refactor #1043

Merged
merged 11 commits into from
Jun 17, 2023
Merged

Route data refactor #1043

merged 11 commits into from
Jun 17, 2023

Conversation

SleeplessOne1917
Copy link
Member

I found the route data somewhat unwieldy to work with. The way it is currently done, any fetchInitialData returns a list of promises, which are then resolved and used as a list in components as isoData. This requires component constructors to make sure that the correct array index is being accessed, and it requires adding promises that do nothing to fetchInitialData.

This change makes it so that each route data is an object with a type. This should hopefully make any isomorphic logic easier to maintain.

@dessalines
Copy link
Member

I'm all for something like this, but its not a good time, right when I'm in the middle of the websocket -> http rewrite. All the files you changed have been reworked pretty heavily, that would make merging this really difficult.

You could rebase this on top of my use_http_client branch, but I wouldn't recommend it yet, because I'm still in the middle of getting everything working.

Lets hold off on this and any other larger changes until after I finish the http stuff, so we don't get stuck in merge hell.

@SleeplessOne1917 SleeplessOne1917 marked this pull request as draft May 30, 2023 10:21
@alectrocute
Copy link
Contributor

Are we out of merge hell territory now? This would be a really nice refactor.

@SleeplessOne1917
Copy link
Member Author

@alectrocute I have to merge the HTTP changes into this branch, but once that's done it should be good to review.

@SleeplessOne1917 SleeplessOne1917 marked this pull request as ready for review June 16, 2023 02:39
@SleeplessOne1917
Copy link
Member Author

@dessalines (and @alectrocute since you seem interested too), the refactor is ready to review.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for going through and typing all these.

Run prettier again, lint is complaining.

Besides that, make sure you tested all these routes on first load. If they worked, feel free to merge.

src/server/index.tsx Show resolved Hide resolved
src/server/index.tsx Outdated Show resolved Hide resolved
@dessalines
Copy link
Member

Lets get this one merged next.

@SleeplessOne1917 SleeplessOne1917 merged commit 56500b6 into main Jun 17, 2023
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