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 relative path loading of plugins #3266

Merged
merged 4 commits into from
Oct 15, 2022
Merged

Conversation

cmdcolin
Copy link
Collaborator

Fixes #3262

It adds two examples of loading a UMD plugin to the volvox config

Firstly, this PR makes umdUrl, an existing method for configuring plugin URLs, always loaded relative to index.html. The issue #3262 describes why this wasn't working with relative paths currently

Secondly, this PR also adds new settings, esmLoc/umdLoc, which resolve the plugin location relative to the current config.json being used, which is similar to other "loc" objects in our config files (e.g. track data file locations like bam, bigwig, gff, etc are assumed to be in the same "folder" as the config.json)

I didn't add cjsLoc (commonjs) yet, used for desktop plugins, but can add it if there is interest

Finally, I added examples to the volvox config using umdUrl and umdLoc. I did NOT use esmUrl/esmLoc because it does not work in firefox with webworkers. firefox cannot load ESM modules in workers currently, though they may add it in the future.

As a result, I added a "minimal" UMD style plugin that can be handwritten. It's more annoying to write this way than ESM but, more browser compatibility

Example minimal plugin


class UMDLocPlugin {
  name = 'UMDLocPlugin'
  install(pluginManager) {}
  configure() {}
}

// the plugin will be included in both the main thread and web worker, so
// install plugin to either window or self (webworker global scope)
;(typeof self !== 'undefined' ? self : window).JBrowsePluginUMDLocPlugin = {
  default: UMDLocPlugin,
}

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 12, 2022
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 12, 2022
@cmdcolin
Copy link
Collaborator Author

also, I removed the strict check for the plugin class being an instanceof Plugin, since i think it is easier to just say that it is a basic class. If needed, we can make the handwritten plugin "extend JBrowseExports["@jbrowse/core/Plugin"]" or something like that to fulfill instanceof but I think it may not truly be needed

import ReExports from './ReExports'
import { isElectron } from './util'

export const PluginSourceConfigurationSchema = ConfigurationSchema(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was unused :)

'umdUrl' in p &&
storePlugin.umdUrl === p.umdUrl)),
p =>
isUMDPluginDefinition(p) &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i didn't add checks for e.g. umdLoc here because since i didn't think our plugin store would use this

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 12, 2022
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #3266 (d1545a8) into main (4018477) will increase coverage by 0.04%.
The diff coverage is 21.56%.

@@            Coverage Diff             @@
##             main    #3266      +/-   ##
==========================================
+ Coverage   59.45%   59.49%   +0.04%     
==========================================
  Files         676      676              
  Lines       28828    28807      -21     
  Branches     7039     7041       +2     
==========================================
+ Hits        17139    17140       +1     
+ Misses      11415    11393      -22     
  Partials      274      274              
Impacted Files Coverage Δ
packages/core/rpc/RpcManager.ts 70.58% <ø> (ø)
packages/core/rpc/WebWorkerRpcDriver.ts 0.00% <0.00%> (ø)
products/jbrowse-desktop/src/StartScreen/util.tsx 0.00% <0.00%> (ø)
products/jbrowse-desktop/src/rpc.worker.ts 0.00% <0.00%> (ø)
products/jbrowse-web/src/rpc.worker.ts 0.00% <0.00%> (ø)
packages/core/PluginLoader.ts 26.43% <31.70%> (+2.26%) ⬆️
products/jbrowse-web/src/SessionLoader.ts 63.92% <40.00%> (+2.62%) ⬆️
packages/core/PluginManager.ts 90.58% <50.00%> (-0.65%) ⬇️
...rative-view/src/ServerSideRenderedBlockContent.tsx 64.00% <0.00%> (-4.00%) ⬇️
products/jbrowse-web/src/util.ts 48.33% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin cmdcolin added bug Something isn't working enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) enhancement New feature or request labels Oct 13, 2022
@cmdcolin cmdcolin force-pushed the fix_relative_path_loading branch 5 times, most recently from 19b9f93 to 61f6447 Compare October 13, 2022 15:46
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Oct 13, 2022

@carolinebridge-oicr I updated the no-build plugin tutorial to reflect some of the lessons learned from this PR, and tagged you for review too.

I changed the no-build tutorial to use a UMD type plugin, and removed the unpkg code since that would use ESM. Perhaps could be added back in the future if ESM gets supported by firefox

@cmdcolin cmdcolin force-pushed the fix_relative_path_loading branch 4 times, most recently from 3a31d2c to 26b0b53 Compare October 13, 2022 16:20
@cmdcolin
Copy link
Collaborator Author

I think this should be good to go...might go ahead with merge

@garrettjstevens
Copy link
Collaborator

also, I removed the strict check for the plugin class being an instanceof Plugin, since i think it is easier to just say that it is a basic class. If needed, we can make the handwritten plugin "extend JBrowseExports["@jbrowse/core/Plugin"]" or something like that to fulfill instanceof but I think it may not truly be needed

Might be good to at least check that the plugin has install and configure methods, then. Other than that, looks good!

@cmdcolin
Copy link
Collaborator Author

cool :) added checks for install and configure at least existing as properties

made it so that 'install' and 'configure' substitute for the instanceof-style check d1545a8 (had done a different thing to detect the different types of 'load' records, but this seems better)

@cmdcolin cmdcolin merged commit c234d2e into main Oct 15, 2022
@cmdcolin cmdcolin deleted the fix_relative_path_loading branch October 15, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin esmUrl may not work in web worker with relative paths
2 participants