Untangle client and server #4861
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The mandatory fields are unset, stop lying to the compiler
There were quite some errors, where the type was passed the wrong way
```
// This is invalid
"change-password": ({ old_password: string, new_password: string, verify_password: string})
// What was actually meant
"change-password": (data: { old_password: string, new_password: string, verify_password: string})
```
The whole callback function is also very verbose as is, with fluff we don't need.
It's always a function that returns void, so there's no real information to be gained
by spelling it out time and time again.
Let's use a helper type that just accepts the payload.
That should make the above error impossible to do.
The sort event bundled networks and channels for no reason at all. They share none of the actual logic, so combining them just makes the typing poor but serves no benefit.
The auth functions are a bloody mess and need to be cleaned up. using various callback functions and using variables as pointers makes the logic hard to follow and hence idiotic to type too, as multiple orthogonal logic paths are mixed up into one function. This really needs to be untangled
userAway is purely server side and we don't send it to the client
This makes the code somewhat ugly, but to properly fix we need to enforce the needed fields
The codebase shoves various things into channel objects to transmit them for things like channel lists etc. This however means that the type does contains the fields and needs to export them. We should clean up the events so that we can get rid of all that. But for now, we adapt the test expectation to reality.
The client side fetches the user list when needed, we don't send it over from the server. Hence modify the test expectation.
Doesn't matter which, code happens to emit undefined. Adapt test expectation over writing strange || null code. The conditional just checks for a falsey value.
The NetworkForm type is wrong, hence the compiler can't infer the type. This needs quite some changes, so for now we just turn the linter off for the 2 watch functions. The whole component is too dynamic to fix easily.
brunnre8
commented
Apr 21, 2024
| // if (this[prop] !== undefined || (Array.isArray(this[prop]) && this[prop].length)) { | ||
| // newChannel[prop] = this[prop]; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Is there any reason that we need to do that dance from the old code there?
I couldn't figure out why it was done that way originally.
There was a problem hiding this comment.
No idea, its been there a long time. I think
This was flagged as an issue by codeQL > Server crash [High] > The server of this route handler will terminate when an > uncaught exception from this location escapes an > asynchronous callback.
MaxLeiter
approved these changes
Apr 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our project was quite confused as to the boundaries between client and server code.
This false sharing meant that it was quite hard to tell what was actually sent to the client and what was uniquely scoped to either side.
Further, this meant that our compilation and build pipelines were very confused and pulled in files they should not have.
This commit series tries to untangle the two. This also entails fixing quite some typing issues.
It's hard to make this in sane, small, commits that still build at each step (it's impossible, as fixing one type error / any type immediately lead to further errors in a game of whack a mole).
So you'll get my actual progress in small commits that can each be reviewed, however the earlier ones are in fact sometimes wrong and get cleaned up later once the picture is a bit clearer.
If you have a better idea of how to do it that isn't just squash it all to a single commit, I'd be all ears