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.

Sign up for GitHub

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

Jump to bottom

feat: remove immutable #497

Merged
merged 5 commits into from
Nov 3, 2023
Merged

feat: remove immutable #497

merged 5 commits into from
Nov 3, 2023

Conversation

pyphilia
Copy link
Contributor

close #433

@pyphilia pyphilia self-assigned this Oct 25, 2023
spaenleh
spaenleh reviewed Oct 26, 2023
View reviewed changes
src/api/chat.ts Outdated Show resolved Hide resolved
spaenleh
spaenleh reviewed Oct 26, 2023
View reviewed changes
src/api/chat.ts Outdated Show resolved Hide resolved
spaenleh
spaenleh reviewed Oct 26, 2023
View reviewed changes
Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

This already looks amazing !

Just a comment, ItemRecord should be replaced with DiscriminatedItem and not Item. We should maybe not export Item outside of its own file in sdk. This could avoid confusion.

@pyphilia pyphilia requested a review from spaenleh October 27, 2023 15:40
@pyphilia pyphilia marked this pull request as ready for review October 27, 2023 15:59
spaenleh
spaenleh requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

Nice work ! There was indeed a lot to change 😓

In hooks we have a lot of .then((data) => data) should we remove them ?

src/api/chat.ts Show resolved Hide resolved
src/hooks/item.test.ts Outdated Show resolved Hide resolved
src/hooks/item.test.ts Outdated Show resolved Hide resolved
src/hooks/item.ts Outdated Show resolved Hide resolved
src/hooks/item.ts Outdated Show resolved Hide resolved
src/ws/hooks/item.ts Outdated Show resolved Hide resolved
test/constants.ts Outdated Show resolved Hide resolved
test/constants.ts Outdated Show resolved Hide resolved
test/constants.ts Outdated Show resolved Hide resolved
test/constants.ts Outdated Show resolved Hide resolved
@pyphilia pyphilia linked an issue Oct 31, 2023 that may be closed by this pull request
Export Chat route has been removed #482
Closed
This was referenced Oct 31, 2023
Closed
Open
@pyphilia pyphilia force-pushed the 433-remove-immutable branch from 7dc46e2 to 8b6e951 Compare October 31, 2023 15:06
@pyphilia pyphilia linked an issue Nov 1, 2023 that may be closed by this pull request
Fix empty CompleteMember for current member #501
Closed
@pyphilia
Copy link
Contributor Author

pyphilia commented Nov 1, 2023

I'm handling kinda the current member issue. I return null instead, which is much better to check the validity of the data.

Also I've removed the onSuccess and onError of some mutations since they are async, I've noticed some returned twice the same toast.

@pyphilia pyphilia requested a review from spaenleh November 1, 2023 12:53
spaenleh
spaenleh approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

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

Changes look good to me, I have left some comments, mostly about cleanup and consistency :) 💪 Great work 🎉

src/api/itemPublish.ts Outdated Show resolved Hide resolved
src/api/itemPublish.ts Outdated Show resolved Hide resolved
src/api/itemPublish.ts Outdated Show resolved Hide resolved
src/api/itemPublish.ts Outdated Show resolved Hide resolved
src/api/member.ts Outdated Show resolved Hide resolved
src/queryClient.ts Show resolved Hide resolved
src/queryClient.ts Outdated Show resolved Hide resolved
src/ws/hooks/item.test.ts Outdated Show resolved Hide resolved
src/ws/hooks/item.test.ts Outdated Show resolved Hide resolved
test/constants.ts Outdated Show resolved Hide resolved
@pyphilia pyphilia force-pushed the 433-remove-immutable branch from acacdb2 to 019b0bd Compare November 3, 2023 09:34
@pyphilia pyphilia force-pushed the 433-remove-immutable branch from 019b0bd to da86a46 Compare November 3, 2023 09:49
@pyphilia pyphilia merged commit b952d4c into main Nov 3, 2023
2 checks passed
@pyphilia pyphilia deleted the 433-remove-immutable branch November 3, 2023 12:01
@github-actions github-actions bot mentioned this pull request Nov 3, 2023
Merged
@github-actions github-actions bot mentioned this pull request Feb 8, 2024
Closed
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spaenleh spaenleh spaenleh approved these changes

Assignees

@pyphilia pyphilia

Labels
v2.0.0
Projects
None yet
Milestone
No milestone
2 participants
@pyphilia @spaenleh