Skip to content

feat(player): Add validation to Player.props.component#820

Merged
JonnyBurger merged 7 commits intoremotion-dev:mainfrom
kaylendog:main
Feb 3, 2022
Merged

feat(player): Add validation to Player.props.component#820
JonnyBurger merged 7 commits intoremotion-dev:mainfrom
kaylendog:main

Conversation

@kaylendog
Copy link
Copy Markdown
Contributor

Simple check to validate Player.props.component is not a <Composition /> element, as rendering this has no visible result. Test included!

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Feb 3, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

remotion – ./

🔍 Inspect: https://vercel.com/remotion/remotion/GhC9Tyd9jc1nwuARN7U5o1RCE1jn
✅ Preview: https://remotion-git-fork-skyezerfox-main-remotion.vercel.app

@kaylendog
Copy link
Copy Markdown
Contributor Author

It would be nice to check the validity of a function with signature () => <Component />, but that has potential side effects, so I don't think it's possible... likewise with lazy components.

@JonnyBurger
Copy link
Copy Markdown
Member

Thanks a lot! Quality!! 🙌

You are right, we cannot test for everything, but I added one more misunderstanding that can happen. Hope it helps someone!

@JonnyBurger JonnyBurger enabled auto-merge February 3, 2022 15:39
JonnyBurger
JonnyBurger previously approved these changes Feb 3, 2022
@kaylendog kaylendog disabled auto-merge February 3, 2022 15:41
@kaylendog
Copy link
Copy Markdown
Contributor Author

kaylendog commented Feb 3, 2022

@JonnyBurger looks like the two commits from the closed PR the other day made it into this one? d044907 and c8078e0. Looks like my forked repo is messy!

@kaylendog
Copy link
Copy Markdown
Contributor Author

@JonnyBurger looks like the two commits from the closed PR the other day made it into this one? d044907 and c8078e0. Looks like my forked repo is messy!

Could've sworn I reset the fork to upstream but apparently not... what do you reckon is the best course of action?

@JonnyBurger
Copy link
Copy Markdown
Member

@SkyezerFox Oh you are right. Very strange, I think it has something to do with me using GitHub Desktop to check it out. It just added two commits to push. Don't know why, but I'll just remove those and then merge it. Thanks for catching this super quickly!

@Iamshankhadeep
Copy link
Copy Markdown
Contributor

this PR is super awesome. thanks @SkyezerFox 🔥

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