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

Add bundler-friendly dependency injection #2627

Merged
merged 2 commits into from
Mar 10, 2020

Conversation

quilicicf
Copy link
Contributor

@quilicicf quilicicf commented Mar 9, 2020

What this PR does

It adds a new dependency injection option that allows one to import a plugin ESM-style and inject it manually in Reveal without the need for fetching another resource.

Example of it in action

import Reveal from './js/reveal';
import zoomPlugin from './plugin/zoom-js/zoom-for-bundler';
import helloWorldPluginCreator from './plugin/helloWorldPluginCreator';

window.Reveal = Reveal;

Reveal.initialize({
  hash: true,
  dependencies: [
    { id: 'zoom', plugin: zoomPlugin, async: true },
    { id: 'hellows', plugin: helloWorldPluginCreator(83, 'S', 'sync plugin'), async: false }, // Loaded from an ESM plugin
    { id: 'hellowa', plugin: helloWorldPluginCreator(65, 'A', 'async plugin'), async: true }, // Can be loaded twice if that makes sense
  ]
});

@quilicicf quilicicf mentioned this pull request Mar 9, 2020
@quilicicf
Copy link
Contributor Author

Damn, a conflict.
I'll fix that during lunch.

@hakimel
Copy link
Owner

hakimel commented Mar 10, 2020

Thanks for this! I'm making some pretty significant changes in the dev branch with the switch to modules so it's hard to avoid conflicts. No worries if you don't have time to fix them—I can handle it when merging.

@hakimel hakimel merged commit 98a6d1d into hakimel:dev Mar 10, 2020
hakimel added a commit that referenced this pull request Mar 10, 2020
@hakimel
Copy link
Owner

hakimel commented Mar 10, 2020

Merged this 🙌

I wonder if we could remove the "id" from the dependency object. Existing plugins already register themselves, and their ID via, Reveal.registerPlugin( 'zoom', RevealZoom ). What do you think @quilicicf?

@quilicicf
Copy link
Contributor Author

Well, I think the fewer fields we have the better but how would you define the plugin's id when registering it via Reveal.initialize({ dependencies: [ { thePlugin } ] }) then? 🤔

@quilicicf quilicicf deleted the dev_importBundledPlugins branch March 10, 2020 12:16
@hakimel
Copy link
Owner

hakimel commented Mar 10, 2020

Never mind. I dug into the code and what I was picturing would not work so let's leave the ID in.

One alternative approach to including bundled plugins that should work is:

Reveal.registerPlugin( 'pluginID', pluginInstance )
Reveal.initialize(...)

@quilicicf
Copy link
Contributor Author

Indeed, but the registration via initialize being documented, I supposed we're forced to support it :-(

R0bes pushed a commit to R0bes/Terraform-Presentation that referenced this pull request Jun 7, 2021
R0bes pushed a commit to R0bes/Terraform-Presentation that referenced this pull request Jun 7, 2021
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.

2 participants