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/reduce plugins scope #41

Merged
merged 20 commits into from
Jan 5, 2022
Merged

Feat/reduce plugins scope #41

merged 20 commits into from
Jan 5, 2022

Conversation

rsanteri
Copy link
Contributor

@rsanteri rsanteri commented Nov 30, 2021

Requirements for a pull request

  • Unit tests related to the change have been updated
  • Documentation related to the change has been updated

Description of the Change

This PR reduces scope of the plugins systems to make it easier to maintain in the future.

Removes slots:

  • run-tab
  • headless

Removes exposed data

  • task
  • run
  • appinfo

This PR also adds some feature to plugin system. Plugin slot will now provide available IDs from current URL to every plugin. Knowing flow_id, run_number or task_id might be useful for the plugin.

Other change this brings is that now plugins needs to define configurations in manifest.json instead of register function. register is replaced with onReady. It is recommended to use onReady to wrap plugin code but not absolute necessity.

Possible Drawbacks

Existing plugins using these slots or data must be updated to work with new API.

plugin-api/Examples/hello-world/plugin.html Show resolved Hide resolved
plugin-api/MetaflowPluginAPI.js Outdated Show resolved Hide resolved
plugin-api/MetaflowPluginAPI.js Outdated Show resolved Hide resolved
src/components/Plugins/PluginManager.tsx Outdated Show resolved Hide resolved
plugin-api/MetaflowPluginAPI.js Outdated Show resolved Hide resolved
@rsanteri rsanteri marked this pull request as ready for review December 13, 2021 16:04
plugin-api/MetaflowPluginAPI.js Outdated Show resolved Hide resolved
Copy link

@skirdey skirdey left a comment

Choose a reason for hiding this comment

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

very few minor things

@@ -36,13 +36,13 @@ A basic plugin could be something like
<script src="MetaflowPluginAPI.js"></script>
<script>
(function () {
Metaflow.register('run-header', () => {
Metaflow.onReady((configuration, resource) => {
Copy link

Choose a reason for hiding this comment

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

below it mentions There are two implemented plugin slots, run-headerandtask-details. The desired slot must be given as parameter to a plugin API register message. but here it removes run-header registry call - is there some equivalent of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to have mention about manifest.json

@@ -59,24 +59,16 @@ This plugin is registered to be rendered in the run header section (path /FLOW_I

## Plugin slots

There are three implemented plugin slots. `run-header`, `task-details`, and `headless`. The desired slot must be given as a parameter to a plugin API register message.
There are two implemented plugin slots, `run-header` and `task-details`. The desired slot must be given as parameter to a plugin API register message.
Copy link

Choose a reason for hiding this comment

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

it should probably mention plugin-api/Examples/hello-world/manifest.json and the definition of the slot for clarity

});
Metaflow.setHeight(100)
Copy link

Choose a reason for hiding this comment

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

trying to understand here why setHeight(100) was replaced by setHeight()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setHeight has optional fixedHeight parameter. When no parameter is given, API will try to find out the full height of document and use that. There wasn't really need for fixed height here.

@@ -1,145 +1,193 @@
const VERSION_INFO = {
api: '0.13.0'
}
api: "0.14.0",
Copy link

Choose a reason for hiding this comment

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

this seems to be a breaking change update, can we do a semantic version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i imagine we should up it to 1.0.0

Copy link

Choose a reason for hiding this comment

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

lets do it and I think we can merge 1.0.0

Copy link
Collaborator

@obgibson obgibson left a comment

Choose a reason for hiding this comment

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

Tests workforme

@obgibson obgibson merged commit bd3bba7 into master Jan 5, 2022
@obgibson obgibson deleted the feat/reduce-plugins-scope branch January 5, 2022 17:23
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