Update profile#561
Conversation
…es, ratings, id), Show clan-search-button if no clan-member
…an"-button is not displayed. (Later maybe a invite-button is displayed if the viewer is a clan-leader.)
|
@geekingfrog I think i have solved everything. |
|
Seems there's still a typechecker problem |
Rimek86
left a comment
There was a problem hiding this comment.
It is to ensure data consistency when the schema evolve. When we update the code/client and the local db is using a previous schema. I think we should keep that even if we can be a bit loose with it for now, and consider only one previous version.
In that case, I'll switch to version 2, which is already on the master branch.
Rimek86
left a comment
There was a problem hiding this comment.
Seems there's still a typechecker problem
I'm not quite sure why yet. I ran “npm run typecheck:node” and “npm run typecheck:web” locally without any errors. What did I do wrong? Do I need to run a different command locally to test it first?
I'll fix it, of course. :-)
"npm run typcheck" is the one i have to execute correct? (Without Node or web..:) |
“typecheck” just runs both, that’s all. We can look at it more closely if it’s still failing. |
|
Clicking "approve launching workflow" only to see red again starts getting tired 😛 |
Also that i think i have everything done... Don't understand why the local check on my client passes... (I'd appreciate any tips on how to prevent this. ) |
Client check result: => @geekingfrog Please start again |
|
|
Don't understand why the results differ |
|
It's clearly because you have |
Ok that make sense but then i can't merge it until tachyon-protocol is min 1.18.2 .... There are my fixes of the clan schema included. |
|
Hum, interestingly I cannot approve to run workflows on this repo. Marek, could you give me this permission please (and for Hectate as well if he hasn't it yet)? Rimek, have you check what version the protocol package is at on npm? Because it's updated rather frequently. |
Why does it matter what is on npm? |
|
Let me just clarify:
|
|
I've switched GitHub actions triggering "Require approval for first-time contributors" to "Require approval for first-time contributors who are new to GitHub" so that should I hope resolve friction. |
|
@p2004a @geekingfrog I merge the upstream master with the current taychon version. Now everything works fine. :-) |
| username: user.username ?? "Unknown User", | ||
| displayName: user.displayName ?? "Unknown User", |
There was a problem hiding this comment.
I think there's a problem there. If you don't have anything in the DB, but you are getting a partial update, then you can't really create the correct record. So either you need to check that the event you got has all the data, that is, it's the initial event and use that. Or, if it's a partial update, you can't use the event and would need to fetchUserInfo to then create the data.
I am not sure how that could happen, but creating a user with these default seems just wrong.
There was a problem hiding this comment.
If you don't have anything in the DB, (...)
The original code just assumes that it can happen and fills in some blanks, no harm done if server functions correctly, but we can make it cleaner.
Or, if it's a partial update, you can't use the event and would need to fetchUserInfo to then create the data.
Not really. Recall the issues with ordering of events/reponses. This would happen here too.
There was a problem hiding this comment.
I think the original author programmed defensively here. I don't think it's wrong to do it that way. It only covers the case where there is inconsistent or incomplete data. It should never happen...
@p2004a How we can make it cleaner? (My goal wasn't to imrpove the users.store.ts My goal was to improve the profiles page here. :-) )
You once asked me not to change too much at once. That's why I wanted to stick to it and not touch everything directly with a PR. Can we possibly make this an issue that will then be changed later?
There was a problem hiding this comment.
@p2004a Hi, i was absent in the last weeks. But now i thin i find time again to continue. What's about my question so that i can continoue here...
There was a problem hiding this comment.
It needs to be changed in protocol to be merge patch too, including all the implications of that: drop all the allowed nulls, it's reserved to patch semantic. (overall, having a optional field that can also take null is weird)
My goal wasn't to imrpove the users.store.ts My goal was to improve the profiles page here. :-)
I mean, piling up more mess on the top is not correct strategy, this is not deep refactoring, you are changing this code.
All this code can be simplified to
const toUpdate = await db.users.bulkGet(event.users.map((u) => u.userId));
const updatedUsers = event.users.map((u, i) => applyPatch(toUpdate[i] || {}, u));
await db.users.bulkPut(updatedUsers);
I've updated the profile page a bit to include the information already available in the Tachyon Protocol, and at the same time, I've improved the design a little. (As a first small step here.)
Updates: