-
Notifications
You must be signed in to change notification settings - Fork 25
(NII-Proposal) add plugin architecture for file actions #841
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
base: develop
Are you sure you want to change the base?
(NII-Proposal) add plugin architecture for file actions #841
Conversation
- Add FileActionExtension for menu/toolbar/both targets - Add ExtensionRegistry service for dynamic extension loading - Add extensions.config.ts for configuration - Add Copy Link as example extension - Add OnlyOffice as example extension (dummy implementation) See docs/file-extensions.md for usage.
ceb269e to
9f95349
Compare
nsemets
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the extension system appears well-designed. I reviewed it as I would a standard PR to ensure everything is in order and to identify any potential issues. I have added some questions and suggestions for consideration. Additionally, I am wondering about translations: are they required in this case?
| async function loadExtension(extConfig: ExtensionConfig, registry: ExtensionRegistry): Promise<void> { | ||
| const module = await extConfig.load(); | ||
| const factory = (module as Record<string, FactoryFunction>)[extConfig.factory]; | ||
|
|
||
| if (typeof factory !== 'function') { | ||
| throw new Error(`Extension factory "${extConfig.factory}" not found in module`); | ||
| } | ||
|
|
||
| const extensions = factory(extConfig.config); | ||
| registry.register(extensions); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s better to add error handling here to be safe.
| export const EXTENSION_INITIALIZATION_PROVIDER: Provider = { | ||
| provide: APP_INITIALIZER, | ||
| useFactory: (registry: ExtensionRegistry) => { | ||
| return () => loadExtensions(registry); | ||
| }, | ||
| deps: [ExtensionRegistry], | ||
| multi: true, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APP_INITIALIZER is deprecated in favor of theprovideAppInitializer function. Use export const EXTENSION_INITIALIZATION_PROVIDER = provideExtensionInitialization();.
| for (const ext of extensionConfig) { | ||
| if (!ext.enabled) continue; | ||
| await loadExtension(ext, registry); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to use parallel loading with Promise.all or Promise.allSettled.
|
|
||
| private actionContext = computed((): FileActionContext => { | ||
| const file = this.file(); | ||
| if (!file) throw new Error('file is required for actionContext'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add a translation key here.
| [class.p-disabled]="item.disabled" | ||
| [attr.aria-disabled]="item.disabled ?? false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it? PrimeNG's TieredMenu should handle disabled items automatically.
|
|
||
| protected actionContext = computed((): FileActionContext => { | ||
| const file = this.file(); | ||
| if (!file) throw new Error('file is required for actionContext'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need translation key.
| () => this.isButtonDisabled() || (this.googleFilePickerComponent()?.isGFPDisabled() ?? false) | ||
| ); | ||
|
|
||
| protected actionContext = computed((): FileActionContext => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| protected actionContext = computed((): FileActionContext => { | |
| actionContext = computed((): FileActionContext => { |
|
|
||
| protected actionContext = computed((): FileActionContext => { | ||
| const folder = this.currentFolder(); | ||
| if (!folder) throw new Error('folder is required for actionContext'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add translation key.
| const extensions = this.extensionRegistry | ||
| .extensions() | ||
| .filter((ext) => !ext.parentId) | ||
| .filter((ext) => !ext.visible || ext.visible(ctx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use only 1 filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it as separate file? Also, need improvements.
Purpose
Add a plugin architecture for file browser actions (context menu and toolbar). Extensions are included in the codebase but can be enabled/disabled at build time via
extensions.config.ts.Summary of Changes
FileActionExtensioninterface for menu/toolbar/both targetsExtensionRegistryservice for dynamic extension loadingextensions.config.tsfor build-time configurationSee docs/file-extensions.md for usage.
Screenshot(s)
Example: Copy Link
extensions.config.ts:
Copy Link (Menu Item):

Example: OnlyOffice Integration (dummy)
extensions.config.ts:
Edit by OnlyOffice(Menu Item/Toolbar Item in the file detail):
Create File (Toolbar Item in the file list):
Side Effects
prebuild,prestart,pretestnpm scripts that generateextensions.config.tssrc/app/extensions.config.tsis now git-ignored (generated fromextensions.config.default.ts)QA Notes
extensions.config.ts