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

refactor: Change lib v2 code to TS #43

Conversation

yusuf-musleh
Copy link

This PR is temp for review, should be replaced with openedx#1124 once base PR is merged.

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/use-ts-for-lib-v2 branch from 9694943 to 5cbff4a Compare June 23, 2024 15:49
Copy link

codecov bot commented Jun 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (yusuf-musleh/sort-filter-v2-libraries@49b7142). Learn more about missing BASE report.

Additional details and impacted files
@@                           Coverage Diff                            @@
##             yusuf-musleh/sort-filter-v2-libraries      #43   +/-   ##
========================================================================
  Coverage                                         ?   92.65%           
========================================================================
  Files                                            ?      712           
  Lines                                            ?    12724           
  Branches                                         ?     2800           
========================================================================
  Hits                                             ?    11789           
  Misses                                           ?      900           
  Partials                                         ?       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/use-ts-for-lib-v2 branch 2 times, most recently from 86f9371 to f0ea105 Compare June 24, 2024 13:36
@yusuf-musleh yusuf-musleh marked this pull request as ready for review June 24, 2024 13:45
@@ -98,7 +98,7 @@ const LibrariesV2Tab = ({
</div>

{ hasV2Libraries
? data.results.map(({
? data?.results.map(({
Copy link
Member

Choose a reason for hiding this comment

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

react-query guarantees that only one of data, isLoading, or isError will be truthy at any given time, so you can either change this to data!.results or refactor the code like this:

if (isLoading) {
  // Display loading state
}
if (isError || data === undefind) {
  // Display an error message. The `|| data === undefined` is only needed for TypeScript to understand the flow.
}

// Render the normal component. Here TypeScript now knows that `data` is not undefined

Copy link
Author

Choose a reason for hiding this comment

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

I see, I've updated it to use the data! and made sure that the to check for !isLoaded in the hasV2Libraries which is used in a bunch of places. (isError is already checked at the beginning and returns): a72713e

Copy link
Member

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 This works as expected, thank you @yusuf-musleh

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/use-ts-for-lib-v2 branch from f0ea105 to 5d4c369 Compare June 25, 2024 15:41
@yusuf-musleh
Copy link
Author

@pomegranited Thanks for the review!

Note there is a bug that linting for .ts[x] files is not happening properly; I'm fixing it in openedx#1129 but please manually make this change and run npm run lint yourself to make sure this PR is good too.

@bradenmacdonald I made sure to run the linter and there are no issues with the file changes in this PR.

@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/sort-filter-v2-libraries branch from 49b7142 to 6888565 Compare June 29, 2024 13:31
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/use-ts-for-lib-v2 branch from a72713e to 05eda0a Compare July 5, 2024 14:31
@yusuf-musleh yusuf-musleh force-pushed the yusuf-musleh/sort-filter-v2-libraries branch from 6888565 to 2469f04 Compare July 5, 2024 14:32
@yusuf-musleh yusuf-musleh changed the title refactor: Change lib v2 code to TS (temp) refactor: Change lib v2 code to TS Jul 5, 2024
@yusuf-musleh yusuf-musleh merged commit 4fd42b5 into yusuf-musleh/sort-filter-v2-libraries Jul 5, 2024
@bradenmacdonald bradenmacdonald deleted the yusuf-musleh/use-ts-for-lib-v2 branch November 6, 2024 19:23
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.

3 participants