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

docs: Add slot naming and life cycle ADR #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arbrandes
Copy link
Contributor

@arbrandes arbrandes commented Dec 19, 2024

This adds an ADR to govern plugin slot naming and maintenance life cycle.

@arbrandes arbrandes force-pushed the slot-naming-and-life-cycle branch from 062da41 to a833874 Compare December 19, 2024 18:56
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (73ab604) to head (c404ef9).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #99   +/-   ##
=======================================
  Coverage   97.81%   97.81%           
=======================================
  Files          10       10           
  Lines         229      229           
  Branches       59       56    -3     
=======================================
  Hits          224      224           
  Misses          4        4           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arbrandes arbrandes force-pushed the slot-naming-and-life-cycle branch from a833874 to c404ef9 Compare December 19, 2024 19:00
@brian-smith-tcril brian-smith-tcril self-requested a review December 19, 2024 19:05
Copy link

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Sounds good to me! Though I'd also propose adding a .beta alternative to .v1 which indicates the plugin slot is unstable and can be changed anytime until stabilized as .v1.

@arbrandes arbrandes force-pushed the slot-naming-and-life-cycle branch from c404ef9 to 6174f34 Compare January 8, 2025 14:43
@arbrandes arbrandes force-pushed the slot-naming-and-life-cycle branch from 6174f34 to cbecb49 Compare January 8, 2025 14:43
@arbrandes
Copy link
Contributor Author

Good idea, @bradenmacdonald! I added an explicit reference to the beta suffix.

@arbrandes arbrandes requested a review from davidjoy January 8, 2025 14:46
@davidjoy
Copy link

davidjoy commented Jan 9, 2025

Okay, so my suggestion here is that we add a "slot type" to the naming scheme between Subdomain and Module:

{Reverse DNS}.{Subdomain}.{Slot Type}.{Module}.{Identifier}.{Version}

The reason for this is that frontend-base has been designed to allow us to provide different types of slots, such as UI, services (logging, analytics, auth), scripts (GA), hooks (events), filters (functions), etc. Currently frontend-base has a robust UI slot system, but extending it to other types of slots will be relatively easy.

Further, the TypeScript types in frontend-base is able to use the existence of "ui" in the slot ID to provide detailed type information for writing slot operations, which helps us keep the operation schema simpler with less boilerplate. Further, it means that the slot itself can enforce the types of operations by virtue of its naming scheme; operators and developers can't accidentally use a slot incorrectly because the type system is hooked into the slot ID itself.

So adding this would provide:

  • Further clarity about what sorts of operations are appropriate in the slot and how it manifests in the code.
  • First class, detailed type safety for operations.
  • Supports future extensions to the slot framework.

And for what it's worth, frontend-base takes advantage of the type safety provided by the naming scheme, and it's really nice. :)

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Jan 9, 2025

{Reverse DNS}.{Subdomain}.{Slot Type}.{Module}.{Identifier}.{Version}

That seems like a perfect spot for it! I'm 100% on board!

Edit: I could see an argument for {Reverse DNS}.{Subdomain}.{Module}.{Slot Type}.{Identifier}.{Version} - I think I still prefer slot type before module but I figured it might be worth discussing.

@davidjoy
Copy link

davidjoy commented Jan 9, 2025

Good question, maybe some examples will help? We can imagine New Relic being added in two parts, the actual service implementation (NewRelicLoggingService) and the New Relic script.

With the slot first:

org.openedx.frontend.service.logging.newRelic.v1
org.openedx.frontend.script.logging.newRelic.v1

Vs:

org.openedx.frontend.logging.service.newRelic.v1
org.openedx.frontend.logging.script.newRelic.v1

I don't know! It's a near thing; I'd likely be happy with either. :)

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Jan 9, 2025

More examples to throw into the fray

Slot first

org.openedx.frontend.ui.layout.footer.beta
org.openedx.frontend.ui.courseware.navigation_sidebar.v2

vs

org.openedx.frontend.layout.ui.footer.beta
org.openedx.frontend.courseware.ui.navigation_sidebar.v2

@davidjoy
Copy link

davidjoy commented Jan 9, 2025

Honestly think it flows better with the module first. (Brian's way)

Put another way, a frontend will have modules which will have slots more than a frontend has slots which have... modules... yeah, I think I agree with Brian now. :)

@brian-smith-tcril
Copy link
Contributor

After seeing the examples I'm definitely on the

{Reverse DNS}.{Subdomain}.{Module}.{Slot Type}.{Identifier}.{Version}

team.

@arbrandes
Copy link
Contributor Author

arbrandes commented Jan 10, 2025

I can see the advantages of both. Since you both argued for module first, let me present two arguments for slot type first: one conceptual, one practical.

Conceptual, from generic to specific: in the frontend domain > there are ui type slots > sprinkled across different modules such as courseware. Frontend is the bigger collection, ui a subset, courseware a sub-subset. The type subset is arguably a bigger collection than the module one.

Practical: having the slot type come first makes it a tad easier to group/find all slots of a given type across the codebase, whether it be via grep/sed or via Github search box: you're searching for "frontend.ui" vs "frontend.*.ui" (not even sure the latter is possible via Github search).

A conceivable counter-argument is that it makes it harder to search for all of a module's slots across the codebase... except that you're not going to be doing that (across the codebase) because all of a module's slots are (presumably) in a single repository.

Operators can subsequently insert plugins into this slot by referencing
"arbitrary_slot_name" in configuration as follows:

pluginSlots: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an example of what this config would look like afterwards? Is only the id changing?

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.

5 participants