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

Added plugin infrastructure #16

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

sebastianhaas
Copy link
Member

@sebastianhaas sebastianhaas commented Jul 12, 2016

See #1 for general information about the idea behind this PR.

I created a plugin infrastructure for OSMD as discussed in #1.

Why

  • We want OSMD to be as flexible as possible.
  • We need to enable external components to interact with OSMD.
  • We probably want people to be able to create extensions on the Classical MusicTech Conference Hackathon.

How

  • I added the basic Event (IEvent) to represent a generic OSMD event that can hold a generically typed argument.
  • The numerous events emitted by OSMD that are available to register on, are described in IEventSource.
  • A third-party/external component must implement the IPlugin interface,
  • and can be registered with the OSMD main class (currently MusicSheetAPI)
  • To see how to use it, take a look at the tests I created:

Obvious questions

Why not move stuff from IEventSource to IPlugin?

Using an interface, the subscribing plugin has to implement and handle all possible events, which can be quite a lot. I turned this around, so the plugin can register on only those events it is interested in.

Why did you upgrade typescript and tslint to a development version?

I needed to use chai-spies for testing the event infrastructure, and due to a limitation in the current typescript version, it is not possible to use the typings for that. tslint is just in a dev channel because it depends on typescript. I would love to remove this limitation before merging this PR and use a stable typescript compiler! typings/registry#545

Why did you create a PluginHolder and did not put all the stuff directly to MusicSheetAPI?

I don't know really. Maybe because it keeps it modular and maybe one day we have a different way to instantiate an OSMD other than via MusicSheetAPI. It also keeps MusicSheetAPI cleaner and more focused on its actual purpose.

Open issues

  • Can we live with a non-stable typescript for a while? What is our alternatives?
  • Think about IEvent (see discussion below)
    - [ ] Code review by @acondolu
  • Code review by @oliver-h

@sebastianhaas sebastianhaas added the enhancement a (small) improvement that neither adds nor fixes a significant feature label Jul 12, 2016
@sebastianhaas sebastianhaas added this to the 0.0.1.alpha.1 milestone Jul 12, 2016
@oliver-h
Copy link
Collaborator

I think the event structure is fine, of course there are always a lot of ways of doing this, but my first impression is that your approach is clean and well structured.

Maybe the IEvent interface is not really needed now (and maybe never will), so one could question it, but by thinking over it again I would stick to it because it keeps us flexible and doesn't hurt us...

I would prefer wait for a stable typescript compiler before merging the pull request, since it could cause some troubles and we don't need it right now. So I would suggest we come back to this in 2 weeks and see if things have changed and discuss it again (and make the final code reviews then).

@sebastianhaas
Copy link
Member Author

Btw, pasted in from #1

I used the following Event architecture:
http://stackoverflow.com/questions/12881212/does-typescript-support-events-on-classes
Problems arised when using chai-spies. typings/registry#545
Had to alter tslintconfig because of palantir/tslint#1205.

Waiting for a stable compiler is fine for me, I just thought you needed this quite soon?
If not, we should change the milestone for #1 and see if we can get the other issues done before Friday e.g. 0.0.1-alpha.1. Need to make a release soon since the current one is broken due to the stupid DefinitelyTyped changes at jszip.

Re IEvent: agreed, this is completely redundant atm. If you want, I can remove it. Let's see before we merge. I added a task to the PR description.

@oliver-h
Copy link
Collaborator

The npm package was much more important and opened the doors for many days of happy coding. We will need the event structure, and hopefully soon, but since I'm alone in the AA project right now (there also popped up some unexpected tasks) and also involved to the other projects, I think we still have some time and can postpone this task for now (as I said, maybe 2 weeks or so)

This was referenced Jul 13, 2016
@sebastianhaas sebastianhaas modified the milestones: 0.0.1-alpha.2, 0.0.1-alpha.1 Jul 15, 2016
@phonicscore phonicscore removed this from the 0.0.1-alpha.2 milestone Aug 23, 2016
@matt-uib matt-uib closed this May 14, 2020
@matt-uib matt-uib deleted the feature/pluginInfrastructure branch May 14, 2020 08:48
@matt-uib matt-uib restored the feature/pluginInfrastructure branch May 14, 2020 09:46
@sschmidTU sschmidTU reopened this May 14, 2020
@sschmidTU sschmidTU force-pushed the develop branch 2 times, most recently from 9b4a2fa to a64b76e Compare March 30, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a (small) improvement that neither adds nor fixes a significant feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants