Initial review

[danyill]

This plugin is now ready for initial review.

It was initially started to resolve https://github.com/openscd/open-scd/issues/1025 but ended up being a (the first non-trivial) plugin to be refactored as an OpenSCD core plugin and the first plugin to depend on scl-lib

• Most of the core functionality is now provided by scl-lib for subscribe, unsubscribe and use of preconfigured types.
• There are (almost) no unit, snapshot or integration tests - everything is tested through visual tests. The coverage of these is probably about 95% in my mind (and not anything like the code coverage formally), but I think it is sufficient for a v1 release.
• It is the work of a hobby developer, rather than a professional, so it is somewhat reluctantly the first plugin to be refactored as a core plugin.
• It has been tested fairly extensively on a range of Ed 2 and Ed 2.1 files.

I don't have enough distance from this code to know where to make improvements and what to do next.

I think the subscriber view is visually cluttered (and would like to improve it but perhaps not for the first release?) and that putting the basic type and CDC on a tooltip wasn't a good design choice (better would have been mock up in https://github.com/openscd/open-scd/issues/1059).

I would like to transition this to the open-scd organisation and would be happy to maintain it there. As I see it a few things need to happen before this can occur:

• A process and set of technical requirements for moving into open-scd.
  • Are we using submodules?
  • What is the release strategy? Will we be automatically updating plugins from open-scd (e.g. latest vX.Y tag) or just on request?
  • What items should be common in our README/documentation to "brand" or "align" core plugins?
• An action-to-edit plugin needs to be merged (prototype already available from Jakob and appears to work well).

I would also like to:

• Contemplate if screenshots need to be added by CI on a PR merge (as per oscd-tree-grid)
• Cancel deployment if tests do not pass in CI
• Automate the release process (perhaps this should be a requirement of an OpenSCD shepherded plugin so that someone who is not maintainer of the plugin can do emergency releases without difficulty and without knowing secrets).
• Add at least a stub CHANGELOG

This review should not be rushed from my point of view and I will be pleased to make changes on request, here are some factors to think about in terms of what this plugin provides and timeframes:

• The existing later binding subscriber plugins have a range of bugs around supervision and other items (see https://github.com/openscd/open-scd/issues/1246, https://github.com/openscd/open-scd/issues/1231, https://github.com/openscd/open-scd/issues/1104).
• There are new features here (https://github.com/openscd/open-scd/issues/1025, https://github.com/openscd/open-scd/issues/1158, partially reoslves https://github.com/openscd/open-scd/issues/1059) (also various issues logged on this project).
• I may be much less available between mid-September and late-October so I'd ideally like to have this completed by then if it was possible.
• We may want to use this as a template for message/LN binding subscription plugins.

Ideally we'd decide after review which changes should be issues for later digestion and which should be done immediately.

After this is incorporated in open-scd, I'm keen to prototype https://github.com/openscd/open-scd/issues/1088 and I hope this plugin has been designed to allow that to happen smoothly.

[danyill]

@ca-d I have done some refactoring as we discussed and I'm hoping you're available to do some initial review and discussion about this plugin. Hopefully the above thoughts are helpful.

[danyill]

Also:

• perhaps downgrade mwc components for custom elements registry issues to use common based with open-scd
• when this plugin is used in a more recent version of core, scrollbars appear and must not :-(

[danyill]

Here is an example file which is not terribly performant with this plugin:

IEC61850_GOOSE_6-section_After_Replace.zip

[danyill]

Regarding https://github.com/openscd/open-scd/issues/1088, I am now quite happy with https://github.com/danyill/oscd-subscriber-lb-siemens except that I think further abstraction of the class method for subscribeand unsubscribe is necessarily for the oscd-subscriber-later-binding plugin to be readily mocked for tests. I'm hoping to do that in scl-lib.

• removeSubscriptionSupervision is this PR
• We still need to agree the API to add subscription supervision see https://github.com/OpenEnergyTools/scl-lib/issues/20

[danyill]

A great deal of the core functionality has been moved into scl-lib which will have improved the basic test coverage no end.

Quite a lot of further abstraction would be possible on the UI side. Some of which has been done by @JakobVogelsang at least to a prototype stage (to allow improved search/cascading hierarchies without very verbose use of mwc-list and mwc-list-item).

The test approach is becoming unwieldy at the same time as the plugin is "good enough" for immediate needs.

I'm fairly confident no review is on the way so I'll de-milestone this for now.

[ca-d]

I'm sorry this got so lost in the mists of other ongoing development. After our development work on other plugins last year, I currently view work on open-scd-core performance (regarding memory use) and features as more valuable than review on any existing plugins. Do you agree?

[danyill]

Yes, for sure 👍

On Tue, 2 Apr 2024, 22:10 cad, @.***> wrote:

I'm sorry this got so lost in the mists of other ongoing development. After our development work on other plugins last year, I currently view work on open-scd-core performance (regarding memory use) and features as more valuable than review on any existing plugins. Do you agree?

— Reply to this email directly, view it on GitHub https://github.com/danyill/oscd-subscriber-later-binding/issues/27#issuecomment-2031469719, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFEXX3OI75LKF6MYLIMDFDY3JYWXAVCNFSM6AAAAAAZ27FUIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZRGQ3DSNZRHE . You are receiving this because you authored the thread.Message ID: @.***>