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

Fix plugin loader for function definitions in window #2186

Merged
merged 3 commits into from
Dec 9, 2020

Conversation

Maxr1998
Copy link
Member

@Maxr1998 Maxr1998 commented Dec 6, 2020

This reverts commit 2dede41 and fixes supporting functions as well as classes as plugin definitions in window.

The previous change breaks plugin loading on Android, as plugin definitions with a function return a Promise to the class (async function), which cannot be instantiated. The Promise needs to be awaited on first; and the instantiation requires the default dependencies as constructor arguments.

@nielsvanvelzen
Copy link
Member

This will fail if pluginInstance is a class because constructing a class needs the new keyword.

See #2157 and #2159

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 6, 2020

But if it is a class, it'll still break because it's missing the arguments/it's calling new twice. We have to fix this differently then..

@nielsvanvelzen
Copy link
Member

Something like this might work, did not test it:
if (typeof pluginInstance === 'function' && !pluginInstance.constructor) {

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 6, 2020

Yes, that could work. I'll try it later and if it does, I'll add it to this PR.

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 6, 2020

Looks like it doesn't, since functions also have the constructor attribute.

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 6, 2020

I just pushed a "fix", the code looks a bit Spaghetti, but should be working. Gotta do some additional testing though.

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 6, 2020

Tested on production with the Android app, and it works just fine 🎉

@Maxr1998 Maxr1998 changed the title Revert "Support plugins as classes" Fix plugin loader for function definitions in window Dec 6, 2020
@dkanada dkanada added the stable backport Backport into the next stable release label Dec 7, 2020
@thornbill
Copy link
Member

I'm not a fan of the spaghetti this adds. Maybe we should just straight revert and not support classes directly? Is it bad if we just say for classes you must return them from a function? window.MyPlugin = () => MyPluginClass

It would be nice if we had a well defined spec for these plugins and what is supported.

@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 7, 2020

As long as there's a documented spec, I'm happy with both options. We just need to decide and document it somewhere.

@nielsvanvelzen
Copy link
Member

Maybe we should just straight revert and not support classes directly?

jellyfin-android does use functions because classes didn't work when I implemented it so I guess we can remove it yes.

@Maxr1998 Maxr1998 marked this pull request as draft December 9, 2020 01:13
@Maxr1998
Copy link
Member Author

Maxr1998 commented Dec 9, 2020

I reverted my spagehtti-ish workaround, as we discussed internally to not support the classes directly in window. Instead, that should be documented and this PR could be merged now. However, I still want to use some changes from my earlier commit (4b7ccc4) to refactor the current code a bit and make it more readable.

@Maxr1998 Maxr1998 marked this pull request as ready for review December 9, 2020 10:56
src/components/pluginManager.js Show resolved Hide resolved
src/components/pluginManager.js Outdated Show resolved Hide resolved
src/components/pluginManager.js Outdated Show resolved Hide resolved
src/components/pluginManager.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@thornbill thornbill merged commit 3992265 into jellyfin:master Dec 9, 2020
@Maxr1998 Maxr1998 deleted the fix-plugin-loader branch December 9, 2020 19:29
joshuaboniface pushed a commit that referenced this pull request Dec 14, 2020
Fix plugin loader for function definitions in window

(cherry picked from commit 3992265)
Signed-off-by: Joshua M. Boniface <joshua@boniface.me>
@joshuaboniface joshuaboniface removed the stable backport Backport into the next stable release label Dec 14, 2020
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.

7 participants