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

Core Typings #348

Merged
merged 6 commits into from
May 23, 2023
Merged

Core Typings #348

merged 6 commits into from
May 23, 2023

Conversation

tymmesyde
Copy link
Member

@tymmesyde tymmesyde commented Mar 14, 2023

Add typings for Core service and models
Not sure if there is a conventional way for where to put / declare them
We could enable checkJs in tsconfig.json to display error warnings and not just auto-completion but it brings lots of errors about undefined components props since we are using prop-type with js

elpiel
elpiel previously approved these changes Mar 15, 2023
Copy link
Member

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

LGTM!

Regarding the prop-types, what other options do we have?
Would it make sense to slowly move towards TS and discard prop-types as dependency?
I guess once we add more typings we could add the option to show errors when a property is missing.

@elpiel elpiel requested a review from sleeyax March 15, 2023 08:48
@tymmesyde
Copy link
Member Author

LGTM!

Regarding the prop-types, what other options do we have? Would it make sense to slowly move towards TS and discard prop-types as dependency? I guess once we add more typings we could add the option to show errors when a property is missing.

I don't think there are other options, switching to ts seems like the next step indeed

sleeyax
sleeyax previously approved these changes Mar 16, 2023
Copy link
Member

@sleeyax sleeyax left a comment

Choose a reason for hiding this comment

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

Is this a first step towards a full typescript migration or is the plan to stick to vanilla JS for a while? Either way, these types are a good improvement towards a better developer experience so I approve :)

@nklhtv
Copy link
Collaborator

nklhtv commented Mar 16, 2023

oh wow, that looks great.
some types are different for each model thogh.
for example the meta items
https://github.com/Stremio/stremio-core-web/blob/d1ff937436820815c14f2e1a27fba065316db772/src/model/serialize_discover.rs#L72
https://github.com/Stremio/stremio-core-web/blob/af20e2b8e79102039b844aa7ad64b43824978067/src/model/serialize_meta_details.rs#L51
i think in board metaItem.videos is undefined
check all the differences, perhaps there are quite a lot, esp with deep links.
If the route does not uses the deeplinks then we dont generate those and deep_links is undefined
declaring .videos as optional and forcing metadetails to check if .videos is not undefined is not a good idea
there should be separate metaItem interface for each route that uses custom schema.
assigning all of the unnessesary props in core is also not a good idea, esp with Stream's deep links. for example we do not use metaItem[x].videos[x].stream[x].deepLinks in the player.

@tymmesyde
Copy link
Member Author

oh wow, that looks great. some types are different for each model thogh. for example the meta items https://github.com/Stremio/stremio-core-web/blob/d1ff937436820815c14f2e1a27fba065316db772/src/model/serialize_discover.rs#L72 https://github.com/Stremio/stremio-core-web/blob/af20e2b8e79102039b844aa7ad64b43824978067/src/model/serialize_meta_details.rs#L51 i think in board metaItem.videos is undefined check all the differences, perhaps there are quite a lot, esp with deep links. If the route does not uses the deeplinks then we dont generate those and deep_links is undefined declaring .videos as optional and forcing metadetails to check if .videos is not undefined is not a good idea there should be separate metaItem interface for each route that uses custom schema. assigning all of the unnessesary props in core is also not a good idea, esp with Stream's deep links. for example we do not use metaItem[x].videos[x].stream[x].deepLinks in the player.

Okay i see
What's the reason for videos being undefined on board and returning MetaItemPreview instead of MetaItem on discover ?
Maybe using something like https://github.com/Aleph-Alpha/ts-rs to generate typings from the core will be more appropriate if there is too much quirks then ?

@nklhtv
Copy link
Collaborator

nklhtv commented Mar 16, 2023

catalog responses return MetaItemPreview[] which does not have videos. Thats the case for both Board and Discover

@tymmesyde tymmesyde marked this pull request as draft March 16, 2023 22:20
@tymmesyde tymmesyde dismissed stale reviews from sleeyax and elpiel via 2483583 March 17, 2023 05:21
@elpiel
Copy link
Member

elpiel commented Mar 17, 2023

Maybe using something like https://github.com/Aleph-Alpha/ts-rs to generate typings from the core will be more appropriate if there is too much quirks then ?

@tymmesyde we could try, we will either need to provide the same types in TS that we need or we can automatically generate from Rust (core and I guess, core-web) and export only the ones that we need in TS.

@tymmesyde tymmesyde marked this pull request as ready for review April 27, 2023 06:07
@jaruba jaruba merged commit 89ae8d5 into development May 23, 2023
@nklhtv nklhtv deleted the typings branch May 25, 2023 11:59
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.

None yet

5 participants