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

PluginCollection logic might be improved #8634

Closed
jodator opened this issue Dec 10, 2020 · 1 comment · Fixed by #8954
Closed

PluginCollection logic might be improved #8634

jodator opened this issue Dec 10, 2020 · 1 comment · Fixed by #8954
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:extending-builds package:core type:refactor This issue requires or describes a code refactoring. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@jodator
Copy link
Contributor

jodator commented Dec 10, 2020

Provide a description of the task

The plugin collection does multiple checks for required plugins but its logic is mangled.

In #8632 I've pushed some improvements in "soft" requirements checking but this whole logic can be changed to two-way pass:

  1. List all plugins to load by inspecting list of plugins and plugins to load from requires property.
  2. Check if all plugins are available (the same as we do it now).
  3. Throw an error if plugins are not found.

This is inspired by the idea of having all the changes in the downcast dispatcher (differ) before running conversion (cc @Reinmar ).

Right now, the logic for which plugin is loaded and which are available is a bit mangled. This will allow checking all dependencies (hard or soft), event if deeply nested (like: ArticlePluginSet -> Essentials -> Enter, etc) before trying to load the editor.

This would also allow to properly order plugins (ie like described in #2907.

Additionally, the errors could describe which plugins are missing and which plugins were requiring it. ATM the errors either say which plugins are missing or which soft requirement was not met (on the first error only).

The thrown errors might need to be updated as well: #8680.

@jodator jodator added type:task This issue reports a chore (non-production change) and other types of "todos". package:core domain:dx This issue reports a developer experience problem or possible improvement. type:refactor This issue requires or describes a code refactoring. squad:dx domain:extending-builds labels Dec 10, 2020
@Reinmar Reinmar added this to the nice-to-have milestone Dec 15, 2020
@psmyrek psmyrek self-assigned this Jan 22, 2021
@Reinmar Reinmar modified the milestones: nice-to-have, iteration 40 Jan 24, 2021
@psmyrek
Copy link
Contributor

psmyrek commented Jan 25, 2021

Current status

Currently, the procedure for collecting and initializing plugins can be divided into two stages. The first stage is creating an PluginCollection instance, where available plugins from builtinPlugins constructor's array and from the editor's context plugins array are passed to the PluginCollection constructor. In the second stage, the init() method is called on the PluginCollection instance, which receives an additional set of plugins and exclusions from the editor configuration (from plugins, extraPlugins and removePlugins arrays), which also must be taken into account. Finally, all plugins found in the requires static property (defined in the plugin constructor) are recursively loaded, but if the required plugin is not available at a given moment, initialization ends with an error. This causes problems, because the success of initializing all plugin instances depends on the order in which they were defined. Additionally, the logic is a bit confusing there.

Solution proposal

My (very general) idea for corrections is as follows:

  • I don't want to change the PluginCollection public API, but just simplify the whole procedure by getting rid of the local state as much as possible. As mentioned above, there are 5 sources, where plugins can be defined and also there is an additional 6th source, which is the exclusion list. All of these stay the same unchanged as the "source of truth" for the PluginCollection.
  • The whole initialization procedure must not depend on the order in which plugins are required and it must validate availability of all deeply nested dependencies before trying to load the editor.

The main place to change will be the init() method in the PluginCollection and the procedure will be based on two stages (as mentioned in the original issue description). In the first stage, all available and "soft" required plugins from the requires array will be recursively collected, but without verification whether the required plugins are available at the given moment. Additionally, the dependencies between plugins will be remembered in order to throw detailed info in case of a possible error. Only after this stage is completed by going through the entire dependency tree, it will be checked if all "soft" required plugins are available. Finally, if no plugin is missing, they all will be instantiated.

pomek added a commit that referenced this issue Feb 8, 2021
Other (core): Improvements and simplifications in the `PluginCollection` logic. Closes #8634.

Fix (core): Added support in the `PluginCollection` to load dependency plugins using soft requirement, if they are not built-in, but they are available further as dependencies of other plugins. See #8634.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:extending-builds package:core type:refactor This issue requires or describes a code refactoring. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants