Skip to content

Conversation

@EthanFreestone
Copy link

@EthanFreestone EthanFreestone commented Mar 21, 2025

Purpose

A recent PR brought 'ui-service-interaction' in as a peer dependency to ui users. In normal operation for FOLIO this would simply resolve itself easily, as both users and ui-service-interaction are parts of the "flower release".

However the intention with NumGen (the feature implemented by UIU-2729) was never intended to bring extra locking dependencies into the frontend and thus entire build process, rather act as an OPTIONAL feature set for modules to implement how they see fit.

Due to constraints when building ui-service-interaction the implementations themselves did not end up being completed by me beyond a very small proof-of-concept, and so some of the architectural nuance ended up being lost.

Approach

I had envisioned something similar to "Pluggable", but instead as a new pattern where modules could surface "public" components to be brought in by other modules should they wish. The "peer dependency" is equivalent to an optional okapi dependency (whichh should ALSO be defined), and thus it should be treated as such.

To that end I propose something akin to Pluggable: ConditonalLoad (Name absolutely up for debate, I am not the best at naming things)

This works utilising Lazy loading to catch imports at runtime, meaning that functionality for flower releases need not be affected, but any implementations of Users without wanting to bring in service interaction and the required interface dependencies of that can do so.

TODOS and Open Questions

Does this belong in stripes-core?
My gut says that such a feature absolutely belongs in stripes core, this PR simply acts as a place to begin that conversation.

Is there anything else required to support this "public" component pattern?
I know that some developers, especially @zburke have had questions about this pattern from the very start. However it has now been ratified by the tech council and so the genie is basically out of the bottle. It is definitely on me to a large extent that the fallout of this design was not thoroughly considered with measures like this put in place, but I believe this 100% MUST be considered for functionality such as UIU-2729 in Trillium in such a key module as users

Pre-Merge Checklist

Before merging this PR, please go through the following list and take appropriate actions.

  • I've added appropriate record to the CHANGELOG.md
  • Does this PR meet or exceed the expected quality standards?
    • Code coverage on new code is 80% or greater
    • Duplications on new code is 3% or less
    • There are no major code smells or security issues
  • Does this introduce breaking changes?
    • There are no breaking changes in this PR.

Added new Component "ConditionalLoad" which can work analogously to Pluggable

This should be considered for stripes-core or otherwise
@github-actions
Copy link

github-actions bot commented Mar 21, 2025

Jest Unit Test Results

    1 files  ±0    268 suites  +2   5m 3s ⏱️ -3s
1 226 tests +8  1 223 ✅ +8  3 💤 ±0  0 ❌ ±0 
1 267 runs  +8  1 264 ✅ +8  3 💤 ±0  0 ❌ ±0 

Results for commit 65eb77d. ± Comparison against base commit b29d250.

♻️ This comment has been updated with latest results.

@EthanFreestone
Copy link
Author

Note... this doesn't seem to work quite as expected currently, I think the approach is still up for debate, but something is breaking WRT the dynamic import of modules in the workspace :(

@EthanFreestone
Copy link
Author

EthanFreestone commented Mar 21, 2025

Of course, the alternative is to use <IfInterface> here, and handle this purely as a peer dep. That might be less disruptive and more palatable?

I thought it worth a conversation though, at least

@sonarqubecloud
Copy link

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.

2 participants