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

Feature: Adds new method to extensionRegistry for appendCondition & appendConditions #2233

Merged

Conversation

warrenbuckley
Copy link
Contributor

@warrenbuckley warrenbuckley commented Aug 22, 2024

Description

Adds two new methods to extensionRegistry to allow developers to append new conditions to an existing extension.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

Its not possible to update an extension such as Workspace Entity Action with a new condition. The current approach requires you to overwrite the extension. Which involves setting the same manifest meta properties and reimplementing an API to perform the same action as the core implementation of the button.

Scenario:
I want to show/hide the Save and Publish button based on a new condition and keep the existing functionality of the button without overhead of trying to overwrite the extension and keeping it in sync with the core implemetation.

How to test?

  • Read over the PR
  • Read the associated tests
  • Run the tests - ensure it passes

Usage

// Add a condition with a specific config to Section2
const workspaceCondition:WorkspaceAliasConditionConfig = {
	alias: 'Umb.Condition.WorkspaceAlias',
	match: 'Umb.Workspace.Document'
};
extensionRegistry.addCondition('Umb.Test.Section.2', workspaceCondition);
const conditions:Array<UmbConditionConfigBase> = [
	{
		alias: 'Umb.Test.Condition.Valid'
	},
	{
		alias: 'Umb.Condition.WorkspaceAlias',
		match: 'Umb.Workspace.Document'
	} as WorkspaceAliasConditionConfig
]

extensionRegistry.addConditions('Umb.Test.Section.1', conditions);

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@warrenbuckley warrenbuckley changed the title Adds new method to extensionRegistry for appendCondition Feature: Adds new method to extensionRegistry for appendCondition & appendConditions Aug 23, 2024
@warrenbuckley warrenbuckley marked this pull request as ready for review August 23, 2024 10:33
@warrenbuckley
Copy link
Contributor Author

Thanks for re-running the tests for me @madsrasmussen
Now I will wait patiently until this gets reviewed or ask for any rework from me

@warrenbuckley warrenbuckley changed the title Feature: Adds new method to extensionRegistry for appendCondition & appendConditions Feature: Adds new method to extensionRegistry for appendCondition, appendConditions, prependCondition and prependConditions Aug 29, 2024
…y wait until the manifest is registered so late extensions will work as well

Seems to work with my hack in an entrypoint and the console logs the extension with the updated condition but its the tests I am struggling to get to work.

As this code is heavily worked from me trying to Google, read RXJS docs and talking with Copilot I am not fully in confident in this approach
@warrenbuckley
Copy link
Contributor Author

Hiya @nielslyngsoe
This has been a real challenge for me today and I am looking forward to your input/feedback on this.

Adds _whenExtensionAliasIsRegistered as a promise so that we basically wait until the manifest is registered so late extensions will work as well.

This seems to work with my hack in an entrypoint file to hack with and the console logs the extension with the updated condition/s but its the tests I am struggling to get to work.

Warning

As this code is heavily worked from me trying to Google, read RXJS docs and talking with Copilot I am not fully in confident in this approach so please be gentle with me 🙈

@warrenbuckley
Copy link
Contributor Author

Needing to take a break from this test that is failing to demonstrate late loaded extension.
Unsure if its my code or just my test implemtation at this point.

Could do with some cleverer people than me from HQ to give me some input/thoughts
@nielslyngsoe @madsrasmussen or @iOvergaard & others...

 🚧 Browser logs:
      About to go KABOOM..

 ❌ Prepend Conditions > allows conditions to be prepended when an extension is loaded later on
      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
        at createTimeoutError (node_modules/@web/test-runner-mocha/dist/autorun.js:1:191490)
        at ds._timeoutError (node_modules/@web/test-runner-mocha/dist/autorun.js:1:195723)
        at node_modules/@web/test-runner-mocha/dist/autorun.js:1:193804

@nielslyngsoe
Copy link
Member

Hi @warrenbuckley you should not be needing to prepend or append. It will give the same result. Currently the system spins up all conditions, the order means nothing. :-)

@warrenbuckley
Copy link
Contributor Author

warrenbuckley commented Sep 1, 2024

@nielslyngsoe I thought order would be important with last in winning surely?

  • Condition A - ✅
  • Condition B - ❌
  • Condition C - ❌

For me I would have thought that the last one setting permitted to false I accept the extension to be loaded so it overrides the previous ones before it.

Or with this way it would prevent the extension as last one is false

  • Condition A - ❌
  • Condition B - ✅
  • Condition C - ❌

or perhaps this I would expect condition C to then make the extension appear

  • Condition A - ❌
  • Condition B - ❌
  • Condition C - ✅

So can you confirm how it works please @nielslyngsoe

Then happy to amend this PR once I fully grasp how conditions work.

Warning

Niels now confirmed that ALL must pass for it to be loaded in as an extension so the order is not important so the prepend stuff is not needed. If any one condition returns false then it won't load

  • Condition A - ✅
  • Condition B - ✅
  • Condition C - ✅

@nielslyngsoe
Copy link
Member

nielslyngsoe commented Sep 2, 2024

@warrenbuckley It works in the way that all the listed conditions has to be valid for the extension to be shown. Therefore the order doesn't matter. :-)

@warrenbuckley warrenbuckley changed the title Feature: Adds new method to extensionRegistry for appendCondition, appendConditions, prependCondition and prependConditions Feature: Adds new method to extensionRegistry for addCondition & addConditions Sep 2, 2024
…ddCondtions

As the order of conditions is not important, as all conditions set on an extensions must be true/valid
Still need to uncomment & reinvestigate the late loading test
@nielslyngsoe nielslyngsoe marked this pull request as draft September 9, 2024 07:34
@warrenbuckley warrenbuckley marked this pull request as ready for review September 9, 2024 13:27
@nielslyngsoe
Copy link
Member

Hi @warrenbuckley

I have refactored your code and written a few extra tests. I'm happy with the solution now.

Notice I went away for the Async approach as it turned out that appended conditions, would be appended late, like two JS cycles, from when a extension manifest got registered.

In other words, a late coming manifest, would be broadcasted and utilized by all subscribers, for then 2 cycles later to get the additional conditions appended. This was a unnecessary round trip, so I refactored so the additional conditions get appended before the manifest goes into the Extension Registry.

I will wait for the v15/dev branch to get up to date with v.14 before we merge. As you can see there is too many changes as part of this PR right now.

@nielslyngsoe nielslyngsoe changed the base branch from main to v15/dev September 12, 2024 20:22
Copy link

Copy link
Member

@nielslyngsoe nielslyngsoe left a comment

Choose a reason for hiding this comment

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

I'm happy now :-)

@nielslyngsoe nielslyngsoe merged commit bb583bd into umbraco:v15/dev Sep 13, 2024
8 checks passed
@warrenbuckley
Copy link
Contributor Author

WHOOP WHOOP 🎉
Off to read the rework you done and see what I could have done better

@warrenbuckley warrenbuckley deleted the feature/extRegistry-appendCondition branch September 13, 2024 09:11
@warrenbuckley
Copy link
Contributor Author

Just so I am clear, this is for V15 only @nielslyngsoe ?

@nielslyngsoe nielslyngsoe changed the title Feature: Adds new method to extensionRegistry for addCondition & addConditions Feature: Adds new method to extensionRegistry for appendCondition & appendConditions Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants