Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

Convert AudiusBackend to typescript#1650

Merged
sliptype merged 9 commits into
mainfrom
sk-audius-backend-port
Aug 1, 2022
Merged

Convert AudiusBackend to typescript#1650
sliptype merged 9 commits into
mainfrom
sk-audius-backend-port

Conversation

@sliptype

@sliptype sliptype commented Jul 26, 2022

Copy link
Copy Markdown
Contributor

Description

Converts AudiusBackend.js to TypeScript

Used the typed DiscoveryApi to specify argument types on many of the functions. This is with the help of pretty crazy SnakeKeysToCamel type helper.

Most of this extra layer will probably be removed eventually in favor of using the sdk, but types will help in the refactoring process

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

How will this change be monitored?

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

@sliptype sliptype requested a review from dylanjeffers July 27, 2022 18:01
@sliptype sliptype marked this pull request as ready for review July 27, 2022 18:01
@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

@dylanjeffers dylanjeffers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dope work

const addTrackImages = <T extends TrackMetadata>(
track: T
): T & { duration: number; _cover_art_sizes: CoverArtSizes } => {
const addTrackImages = (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice simplifications

function* fetchDashboardListenDataAsync(action) {
const listenData = yield call(
AudiusBackend.getTrackListens,
action.period,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wow.. nice find

userId: currentUser.user_id,
handle: currentUser.handle,
recipientEthAddress: currentUser.wallet,
recipientEthAddress: currentUser.wallet as string,

@dylanjeffers dylanjeffers Jul 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just checking we dont want an if statement to check that case wallet, endpoints, and parallelization are not around?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah actually I think I can add it to the null check above

try {
yield call(waitForBackendSetup)
const accountUserId: Nullable<ID> = yield select(getUserId)
const accountUserId: ID = yield select(getUserId)

@dylanjeffers dylanjeffers Jul 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we yield* these or would that take too long? from what i remember this actually is a Nullable<ID>

@dylanjeffers dylanjeffers Jul 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah just tried it and after changing 'redux-saga/effects' import to 'typed-redux-saga/macro', this logic works:

    const accountUserId = yield* select(getUserId)
    const userMetadata = yield* select(getAccountUser)
    if (!accountUserId || !userMetadata) return

    yield put(
      requestConfirmation(
        DEACTIVATE_CONFIRMATION_UID,
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sick ok will do that instead

const { blockHash, blockNumber } = yield call(
AudiusBackend.updateCreator,
{ ...userMetadata, is_deactivated: true },
{ ...userMetadata, is_deactivated: true } as User,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(and wont need to cast here)

return feedItems.map((item) => {
if (item.playlist_id) return AudiusBackend.getCollectionImages(item)
return feedItems.map((item: Collection | Track) => {
if ((item as Collection).playlist_id) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think you can do if ('playlist_id' in item) which then tells the rest of the branch item is Collection, and then also do we need to type (item: Collection | Track) or can that come from feedItem's type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, yeah that all makes sense

static async searchTags({
searchText,
minTagThreshold,
query,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah looks like it was changed, nice

return true
} catch (err) {
console.error(err.message)
console.log(getErrorMessage(err))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getErrorMessage so clutch

referrer = null,
feePayerOverride = null
}: {
email: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might be nice to create standalone type for this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree but I think I'm gonna leave this for now because the type would have to live all the way at the top of the file outside of the AudiusBackend class. The benefit of having it colocated with the function outweighs the separate type imo here. Can def fix once AudiusBackend is split up and potentially not a class anymore

feePayerOverride,
isFinalAttempt
}: {
challenges: { challenge_id: ChallengeRewardID; specifier: string }[]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and here too potentially

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

@sliptype sliptype requested a review from dylanjeffers July 29, 2022 16:53
@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/sk-audius-backend-port

.map((track: Track) => track.track_id)
.filter(Boolean)

const tracks = lineupItems.filter(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

love it

const accountUserId = yield* select(getUserId)
const userMetadata = yield* select(getAccountUser)
if (!accountUserId || !userMetadata) return
yield* put(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice

@sliptype sliptype merged commit 3e2b499 into main Aug 1, 2022
@sliptype sliptype deleted the sk-audius-backend-port branch August 1, 2022 15:18
audius-infra pushed a commit that referenced this pull request Aug 6, 2022
[c3682b3] [C-780] Update AudiusBackend to work in native context (#1677) Dylan Jeffers
[398093d] [C-781] Add minimum threshold to common password check (#1684) Kyle Shanks
[5a66847] Ensure fetch calls are awaited so errors are caught (#1683) Sebastian Klingler
[d597054] Make audiusBackend configurable for different environments (#1670) Sebastian Klingler
[b5fa3fa] Fix native build with new metro-config dependencies (#1676) Dylan Jeffers
[408a425] [C-771] Update native metro-config to enable @audius/sdk (#1671) Dylan Jeffers
[afde38e] [PAY-416] Early access mode & feature flag refactor (#1654) Michael Piazza
[3b28c7e] Fix typecheck for mobile after AudiusBackend ts migration (#1672) Sebastian Klingler
[e5fbbb5] Revert "Revert "Update explore endpoints in libs to use v1 (#1539)" (#1609)" (#1669) Joseph Lee
[237784b] Default endpoints to empty array instead of null (#1665) Marcus Pasell
[175e5d6] Add ModalContentPages to stems (#1659) Marcus Pasell
[fdadaac] fix mobile menu on narrow (#1664) nicoback2
[3e2b499] Convert AudiusBackend to typescript (#1650) Sebastian Klingler
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants