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

feat: update schema, add provider key + transform #113

Merged
merged 6 commits into from
Nov 29, 2023

Conversation

luwes
Copy link
Collaborator

@luwes luwes commented Nov 22, 2023

  • this pr updates the asset schema to the schema below.
  • poster and sources are derived data so I didn't add them to the stored state yet.
    to generate this derived data, transformer's were added alongside the provider files.
    not in the same file as the provider, because the front-end needs to pull this in.
  • I tried to add some more logic to assets.ts and re-use it where was possible.
export interface Asset {
  status: 'sourced' | 'pending' | 'uploading' | 'processing' | 'ready' | 'error';
  originalFilePath: string;
  provider: string;
  providerSpecific?: {
    [provider: string]: { [key: string]: any }
  };
  blurDataURL?: string;
  poster?: string;
  sources?: AssetSource[];
  size?: number;
  error?: any;
  createdAt: number;
  updatedAt: number;

  // Here for backwards compatibility with older assets.
  externalIds?: {
    [key: string]: string; // { uploadId, playbackId, assetId }
  };
}

@luwes luwes self-assigned this Nov 22, 2023
@luwes luwes requested review from mmcc and heff November 22, 2023 20:36
Copy link
Contributor

@mmcc mmcc left a comment

Choose a reason for hiding this comment

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

This looks good to me! My only thought: do we need/want the CLI to migrate existing assets over? Even if it's not a big deal now, do we want to set the precedent that we'll do so?

src/cli/sync.ts Outdated Show resolved Hide resolved
src/providers/mux/provider.ts Outdated Show resolved Hide resolved
@luwes
Copy link
Collaborator Author

luwes commented Nov 27, 2023

do we need/want the CLI to migrate existing assets over? Even if it's not a big deal now, do we want to set the precedent that we'll do so?

it crossed my mind as well, I thought to keep it for the v1.
externalId's is used only in the transformers as a fallback so it's pretty minimal code to stay backwards compatible

Copy link
Contributor

@heff heff left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 (without fully running and testing it)

@@ -0,0 +1,2 @@
export * as mux from './mux/transformer.js';
export * as vercelBlob from './vercel-blob/transformer.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we get up to 10+ providers is there gonna be any value int splitting them out into their own packages rather than baked in? Or should we always just bake them in since there's a finite amount, to keep our lives easier? No change needed here, just wondering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good point, we might have to split them up eventually because of the added dependencies that they bring.

@@ -133,6 +126,15 @@ export function getVideoProps(allProps: VideoProps, state: { asset?: Asset }) {
return props;
}

function transform(asset: Asset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I want to keep pushing on the idea of transforming the asset before it reaches <Video>. import awesomeVideo could include functions for getting dynamic URLs etc, not just metadata. Then I think the next-video UI layer becomes fully optional and you could just do <video src="${awesomeVideo.src}">. But I know the crux of that is remote URLs, so a bigger conversation.

@luwes luwes merged commit 540e705 into muxinc:main Nov 29, 2023
@luwes luwes deleted the update-schema branch November 29, 2023 15:55
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