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

Upstream types from query-client and use a new magic type for deep casting #75

Merged
merged 23 commits into from
Feb 16, 2023

Conversation

spaenleh
Copy link
Member

This PR adds a frontend entry-point that exposes the types used by the frontends (derived from backend types with immutable sugar coating).

closes #74

@spaenleh spaenleh added the feature New feature or request label Feb 13, 2023
@spaenleh spaenleh self-assigned this Feb 13, 2023
@spaenleh spaenleh linked an issue Feb 13, 2023 that may be closed by this pull request
Copy link
Contributor

@pyphilia pyphilia left a comment

Choose a reason for hiding this comment

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

Amazing work thank you, this change will definitively improve our whole codebase, and I hope it will also improve the frontend's build problems 😍

src/services/items/interfaces/item.ts Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
@spaenleh
Copy link
Member Author

An issue I have when using the ImmutableCast is that the resulting type alias is evaluated to any... This is a problem. I will try to check if using a typing function could work better in this case.

@spaenleh spaenleh force-pushed the 74-afine-item-type-with-discriminated-union branch from b4969f5 to 511316b Compare February 14, 2023 12:47
@spaenleh spaenleh force-pushed the 74-afine-item-type-with-discriminated-union branch from 21f8d8a to 8d53169 Compare February 14, 2023 13:11
@spaenleh spaenleh force-pushed the 74-afine-item-type-with-discriminated-union branch from 4268cec to ba7d1ca Compare February 15, 2023 07:46
@spaenleh
Copy link
Member Author

The issue I faced with types resolving to any was linked to path aliases no beeing resolved to relative urls after transpilation. This was resolved by using tsc-alias and changing the build command to tsc && tsc-alias 😄

@spaenleh spaenleh requested review from pyphilia and codeofmochi and removed request for codeofmochi February 15, 2023 09:15
Copy link
Contributor

@codeofmochi codeofmochi left a comment

Choose a reason for hiding this comment

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

Overall LGTM 🦖 Thank you for your work! I think it's really nice to let the type system do the heavy lifting instead of writing boilerplate code and having weird casts 😄

I've left a bunch of comments, let me know what you think or if you have any question. Not all of them require changes, feel free to decide on what's relevant

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
src/constants/itemLogin.ts Show resolved Hide resolved
src/frontend/types.ts Outdated Show resolved Hide resolved
src/services/chat/types.ts Outdated Show resolved Hide resolved
src/services/items/interfaces/item.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/utils/error.ts Outdated Show resolved Hide resolved
Comment on lines +16 to +20
export const getFileExtra = <
U extends LocalFileItemExtra | ImmutableCast<LocalFileItemExtra>,
>(
extra?: U,
): U[ItemType.LOCAL_FILE] | undefined => extra?.[ItemType.LOCAL_FILE];
Copy link
Contributor

Choose a reason for hiding this comment

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

Very cool, I didn't know that T and immutable.Record<T> would resolve to the same value type when using the same property access pattern (though it makes sense) 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it allows the use of this method in immutable and non-immutable world, with the output being immutable when the input was and vice-versa (not sure it was what you meant 😄 ). This was allowed by the work that we did to be able to access properties of records without the .get() method (still a mystery to me to this day).

@spaenleh spaenleh merged commit e21d1e5 into main Feb 16, 2023
@spaenleh spaenleh deleted the 74-afine-item-type-with-discriminated-union branch February 16, 2023 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Afine Item type with discriminated union
3 participants