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

[C-551] Bundle and init @audius/sdk #3170

Merged
merged 33 commits into from
Jun 3, 2022
Merged

Conversation

sliptype
Copy link
Contributor

@sliptype sliptype commented May 27, 2022

Description

  • Add rollup config to bundle sdk & libs in the following ways:

    • main - Includes sdk and libs exported for a node environment

    • browser - Only includes sdk. Bundled for a browser environment (includes polyfills/shims for dependencies)

    Bundlers such as webpack 5 will automatically use this entrypoint

    • legacy - Includes sdk and libs bundled for a browser environment but without polyfills

    This is for use in audius-client where we are using webpack 4 which will polyfill deps for us. We want both sdk and libs without the added bundle size of including the browser entrypoint

  • Updates services to use @audius/sdk instead of @audius/libs

  • Moves the oauth changes into the sdk

An example of current sdk usage:

import { sdk } from "@audius/sdk";

const initAudiusSdk = async () => {
  const audiusSdk = await sdk({ appName: 'test' })

  audiusSdk.oauth.init(
     () => { console.log('success') },
     () => { console.log('failure') }
  )

  const tracks = await audiusSdk.discoveryNode.getTracks()
};

Tests

Libs tests pass, smoke test passes. Able to upload and view tracks

Copy link
Contributor

@dylanjeffers dylanjeffers left a comment

Choose a reason for hiding this comment

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

this is really coming together love it!!! so lucky that everything you needed had ts already, feels good

@@ -1,23 +1,14 @@
{
"name": "@audius/libs",
"version": "1.2.117",
Copy link
Contributor

Choose a reason for hiding this comment

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

so is the goal to migrate all of our lib consumers to audius/sdk? epic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's the plan, people can either stay on deprecated @audius/libs or update to @audius/sdk and use the libs in there.

An issue with this is that the browser bundle doesn't include libs, so if there are people using libs in the browser right now they will have to use the legacy bundle. But that should work for them if they are successfully using it already

]
}),
alias({
entries: [{ find: 'stream', replacement: 'stream-browserify' }]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is dope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually an interesting one bc the rollup node polyfill for stream has circular dependencies the break when subsequently bundled in webpack. So had to use a different polyfill essentially

alias({
entries: [{ find: 'stream', replacement: 'stream-browserify' }]
}),
nodePolyfills(),
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is helps for the 3rd parties that assume node env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah for example webpack 5, which is used by newest CRA, doesn't polyfill node. But I didn't want people to have to use craco or eject just to use our sdk. It also presents a problem because things can only be polyfilled in internal (bundled) deps, so bundle size quickly spirals out of control if we include libs in this entrypoint

Copy link
Contributor

Choose a reason for hiding this comment

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

cool lots to think about here, great stuff

// For the browser bundle, these need to be internal because they either:
// * contain deps that need to be polyfilled via `nodePolyfills`
// * are ignored via `ignore`
const internal = [
Copy link
Contributor

Choose a reason for hiding this comment

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

browserInternal maybe?

...commonTypeConfig
},

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome comments all-around!

@@ -0,0 +1,102 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

love that this can just be ts from the get-go


discoveryProviderEndpoint: string | undefined

constructor({
Copy link
Contributor

Choose a reason for hiding this comment

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

nice update to constructor

@@ -5,7 +5,7 @@ import {
} from 'ipfs-unixfs-importer'
import fs from 'fs'
import { hrtime } from 'process'
import { promisify } from 'util'
import promisify from 'pify'
Copy link
Contributor

Choose a reason for hiding this comment

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

just using a 3rd party here for correctness and maintainability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I think I can remove this now, was trying this because of some issues with the polyfill

@@ -4,7 +4,7 @@ import { pack } from '@ethersproject/solidity'
import type Web3 from 'web3'

export const sign = (digest: unknown, privateKey: Buffer) => {
const buffer = toBuffer(digest)
const buffer = toBuffer(digest as any)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just make digest any in params if there is nothing we can do?

if (typeof window === 'undefined') {
// TODO(nkang): Add link to documentation once written
throw new Error(
'Audius OAuth SDK functions are only available in browser. Refer to our documentation to learn how to implement Audius OAuth manually.'
)
}
this.discoveryProvider = discoveryProvider
this.appName = null
this.appName = appName ?? null
Copy link
Contributor

Choose a reason for hiding this comment

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

The app name can't be optional because the Oauth popup requires it - can we make it required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yup, will update!

Copy link
Contributor

@nicoback2 nicoback2 left a comment

Choose a reason for hiding this comment

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

Looks great, amazing work! Just left a comment about the appName being required for Oauth

@sliptype sliptype merged commit 5c94b3d into master Jun 3, 2022
@sliptype sliptype deleted the sk-c-551-bundle-init-sdk branch June 3, 2022 19:26
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[eb23ea0] [C-2309] Fix track title hover state (#3197) Dylan Jeffers
[b82a6ee] [C-2303] Ensure consistent play/active state for entity tiles (#3195) Dylan Jeffers
[3faedfb] [C-2223] Flex on Finnuh (#3196) Raymond Jacobson
[d1cd884] [C-2271] Update data passed to edit playlist form to update playlist properly (#3183) Kyle Shanks
[7837460] [C-2187] Improve sign-up error messages (#3194) Dylan Jeffers
[e200477] [C-2287] Update wallet connect language, flags, icons (#3192) Dylan Jeffers
[cc0b56d] allow create notification to handle playlist id as array or int (#3190) sabrina-kiam
[d357b3b] [C-2431] Add tastmaker notification to mobile (#3191) Dylan Jeffers
[83a6d36] No playlists text fix C-2296 (#3189) nicoback2
[0e44281] [PLAT-847]: Fix user subscription create album notification copy (#3162) sabrina-kiam
[7da3e10] [C-2412] Opaque id deeplinks (#3188) Sebastian Klingler
[4ff1c53] [C-2418] Fix mobile web open in app banner (#3173) Raymond Jacobson
[22a1240] [C-1504] Prevent user from following themselves (#3187) Dylan Jeffers
[465182a] [C-2306] Fix sign-in sign-up invalid email bug (#3186) Dylan Jeffers
[2cec764] [C-1750] Refactor top tags (#3180) Dylan Jeffers
[7ca864f] Fix Snap sharing with weird track names C-2081 (#3176) nicoback2
[761187a] Fix missing track on Sammie's page C-2342 (#3182) nicoback2
[41c9cf5] Fix track upload cover art name (#3138) Theo Ilie
[cfb0b96] [C-2428] Handle null in cache map handle/permalink adds (#3184) Andrew Mendelsohn
[0559902] [PAY-1127] Improve mobile chats performance: remove index dep (#3170) Reed
[961649a] Use RNGH touchables for mobile chats (#3174) Reed
[be5d2a5] [C-2427] Fix preinstall checks for ruby and python versions (#3179) Andrew Mendelsohn
[48f8d4c] Update readme with deps, remove sudo (#3178) Reed
[fe79400] Disallow sharing premium content to social media, remove feature flags C-2334 C-2420 (#3172) nicoback2
[776596f] [PAY-829][PAY-1044] Desktop: Chats search users modal permissions/blocks and profile page permissions/blocks (#3156) Marcus Pasell
[e4df304] Fix scrolling on ChatListScreen (#3171) Reed
[6f35699] [PAY-1041][PAY-1040][PAY-1126] Mobile search users screen improvements (#3159) Reed
[d9e302b] [PAY-1143] Desktop: Overflow menu on Chat Header (#3169) Marcus Pasell
[798a664] Fix edit profile button not working after you submit the first time (#3163) nicoback2
[1d78774] [PLAT-814] Use sdk discovery-node-selection in libs (#3125) Dylan Jeffers
[58005c4] [PAY-799] DMs: Link previews (#3096) Marcus Pasell
[7e3d44c] [PAY-924][Desktop] DMs: Blocking/Unblocking Wire Up/UI (#3155) Marcus Pasell
[7038d81] [C-2347] Improve popover zIndex (#3168) Dylan Jeffers
[3eb62fe] [C-2364] Fix missing artist-card badges on android (#3165) Dylan Jeffers
[64ffb7f] Update @audius/sdk to 2.0.3-beta.4 (#3160) Marcus Pasell
[6e5306c] [QA-443] Fix cache case matching for handles and permalinks (#3167) Andrew Mendelsohn
[84dcb5a] [C-2059] Show toast when email check fails due to blocked user (#3166) Sebastian Klingler
[be855a4] [C-2419] Fix offline lineup loading (#3164) Andrew Mendelsohn
[920e25b] Fix permissions message for adding to photos C-2379 (#3161) nicoback2
[78cbdfa] [PLAT-819]: Fix copy for repost and save of albums notifications (#3151) sabrina-kiam
[10617a2] Mobile chats use push instead of navigate (#3154) Reed
[6131075] [C-2406] Fix repeated edits of same track (#3157) Sebastian Klingler
[3ae27d1] Mobile chats scrolling improvements (#3153) Reed
[e820e3f] [C-2336, C-2388] Playlist style and bug fixes (#3149) Kyle Shanks
[955887f] [C-2406] Null check localStorage (#3150) Sebastian Klingler
[3b31b73] [C-2227] Clear wallet state on sign-out (#3147) Dylan Jeffers
[91940fb] [C-2406] Prevent non finite set of currentTime (#3148) Sebastian Klingler
[d799e30] [C-2404] Fix ios build on xcode 14.3 (#3146) Dylan Jeffers
[186a086] [C-2398] Clean up playlist-update feature flags (#3145) Dylan Jeffers
[edb3142] update to xcode 14.2.0 (#3107) Raymond Jacobson
[4add953] [PLAT-835] Fix tastemaker twitter button (#3144) Dylan Jeffers
[e68ada9] [PLAT-834]: Fix notifications last seen new tag (#3143) sabrina-kiam
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants