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

Allowing other plugins to extend views? #123

Open
gakada opened this issue Nov 29, 2018 · 7 comments
Open

Allowing other plugins to extend views? #123

gakada opened this issue Nov 29, 2018 · 7 comments

Comments

@gakada
Copy link
Contributor

gakada commented Nov 29, 2018

To give some context, there is KC3Kai/KC3Kai#2709, which is mainly a submitter for fit tests, but it also has some UI. While submitter is being added via poi-plugin-tsundb, I'm wondering if it can be possible to let that (or any other) plugin to extend views of the main program or prophet plugin (probably extending prophet is more suitable in this case, since fit tests, sorties and prophet plugin are all related).

For example, a working solution for this case is simply adding something like

window.prophetShipViewTooltipExtra && window.prophetShipViewTooltipExtra(data)

in https://github.com/poooi/plugin-prophet/blob/master/views/ship-view.es#L158, allowing other plugins to define (or extend) that function (of type data -> list of React elements).

E.g.

u

Maybe there is more idiomatic way of doing this.

Another example is poooi/poi#1839 in its initial form, that is, allowing plugins to add more info in ship tooltips, such as info based on experience FCD.

@KagamiChan
Copy link
Member

It seems to me the main concept is being able to show some custom information elsewhere than plugin's own area.

I would prefer a more generalized solution for this sort of features, for example, allowing plugin exposing a panel on main tab

@gakada
Copy link
Contributor Author

gakada commented Nov 29, 2018

Well, prophet plugin already provides sortie viewing, so I was thinking maybe it can be used by the fit testing plugin to display its relevant info during sorties. That is without re-implementing something prophet-like again. Plugin area then can be used for something else, like a list of available tests to perform.

Similarly, in the main program there is already tooltips for ship/admiral experience/ranking points, so for users, it would be convenient to have more info here (for example, like ranking points tracking in ElectronicObserver), which a plugin can provide, if those tooltips have extensible areas.

So, in general, it is about extending existing components with existing ergonomic and logic, adding extra panels can require re-implementing the logic or duplicating already existing elements.

@gakada
Copy link
Contributor Author

gakada commented Nov 29, 2018

Btw, with two functions defined, usage can be something like this, in the main program or extensible plugin:

          <ItemView item={data.poi_slot_ex} extra label="+" warn={false} />
          {usePluginComponents('poi-plugin-prophet ShipView Tooltip', data)}
        </div>

in extending plugins

addPluginComponent('poi-plugin-tsundb', 'poi-plugin-prophet ShipView Tooltip', (data, key) => (
  <div key={key}>
    ...
  </div>
)
// etc. to add more
const ensureArray = x => (Array.isArray(x) ? x : x ? [x] : [])

const usePluginComponents = (on, data) => window[on] && window[on](data)

const addPluginComponent = (from, to, component) => {
  if (window[`${from} ${to}`]) {
    return
  }
  window[`${from} ${to}`] = true
  const prevComponent = window[to]
  window[to] = data => {
    const prevElements = ensureArray(prevComponent && prevComponent(data))
    const currElements = ensureArray(component(data, prevElements.length))
    return prevElements.concat(currElements)
  }
}

@KagamiChan
Copy link
Member

Let me explain my concerns on generality about your proposal

  • This API introduce dependencies between plugins, which could become problem with future breaking changes (e.g. prophet choose to pass different data or tsundb requires additional data as parameter)
  • Using window to share data is not considered a good pattern and might break because plugins are loaded concurrently
  • This API's usage is more or less limited (maybe I missed some possibilities with this API)

I appreciate the idea you give and I completely understand that this feature could be useful for many people, but IMO this is something too tricky to implement。 Maybe we could explore further for a better solution of this kind of API

@gakada
Copy link
Contributor Author

gakada commented Dec 6, 2018

  • True, I guess extending plugins can only return elements if all required fields are present in data, otherwise returning null so it doesn't do anything. Though that doesn't account for type and semantic changes.
  • Also true, I thought maybe it is a loader issue, that is, it should sort plugins by priority from package.json/etc. first and then load them sequentially. Also I was thinking maybe redux store can be used instead of window. Also seems like it will only affect order of elements (not an issue if it is single source to single target or if order doesn't matter anyway).

The API is basically

// Call in extensible plugin or main program to pull all elements from registered components from extending plugins
// targetKey is unique per extensible "place"
pull(targetKey: string, data: any): Element | Element[] | null

// Call in extending plugins to register components to add elements into targetKey place
// maybe sourceKey is optional, but maybe it is required to prevent re-adding components
push(sourceKey: string, targetKey: string, component: (data: any, firstKey: Number) => Element | Element[] | null): void

Have to decide that some "place" can pull external elements from plugins and use pull function there, so the place is extensible. Then push those elements in plugins via (pure) components with push function, so the place is being extended.

That is an API with low implementation cost, not sure if there can be more powerful API while still having low implementation cost.

@gakada
Copy link
Contributor Author

gakada commented Dec 6, 2018

For concurrency issues, I'm loading prophet and then loading tsundb, the tooltip thing is still being added 🤔

So maybe the only effect here is if plugins A and B are trying to extend a place, will be either A elements followed by B elements in that place or the other way around. Can be controlled with some package.json dependency filed, also with some handling for plugin reloading (the above addPluginComponent is only working once).

@gakada
Copy link
Contributor Author

gakada commented Dec 6, 2018

Also for compatibility, targetKey can be composed not just with package name and place designator but also with some version, so that any breaking data changes can change that version, so that components for previous versions don't apply.

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

No branches or pull requests

2 participants