-
Notifications
You must be signed in to change notification settings - Fork 156
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
Introduce Skeleton-Application, Runtime hooks and demystify runtime boot logic #5752
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
Results for oCISSharingPermissions2 https://drone.owncloud.com/owncloud/web/18712/58/1 |
Results for oCISSharingPermissions2 https://drone.owncloud.com/owncloud/web/18713/58/1 |
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.
Looks pretty awesome! Not sure how we should proceed here (before you're going on holidays next week), maybe it makes sense to pull the runtime/boot-refactoring out of this PR in a separate one already, so the cognitive load of this one shrinks plus I think it also helps with the Vue3 upgrade
introduce runtime api and use it to lazy register extensions from remote applications
9bd12a2
to
5b91d37
Compare
in the proccess of adding a skeleton application we faced the issue of not having real runtime hooks. this introduces 3 hooks and exposes a dedicated runtime api to the application. this is fully backwards compatible
5b91d37
to
dad1a0a
Compare
Results for oC10iPhone1 https://drone.owncloud.com/owncloud/web/18834/45/1
|
Results for oC10Files1 https://drone.owncloud.com/owncloud/web/18837/12/1
|
|
||
const applicationRoutes = routes.map(applicationRoute => { | ||
if (!isObject(applicationRoute)) { | ||
throw new ApiError("route can't be blank", applicationRoute) |
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.
Do we want to force the extension to have routes? What if we'd have some extension that is supposed to do only background jobs? Maybe something not to look into now but for the future 🤷
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.
in the current version of the "application-manifest" we force to use routes. Int he class based which isn't exported right now the user has to call that api it's wown. So if he calls it without providing routes... it errors, if he skip it all ist fine.
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.
Found one important thing: configuring an application in the config.json which is not reachable causes the full runtime to stop with a config error page. Extension loading should fail silently (with console error logging, but not by crashing the whole platform). Some other remarks in the comments.
} | ||
|
||
/** application quick action describes a application action that is used in the runtime */ | ||
export interface ApplicationQuickActions { |
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'd prefer declaring the quick actions in singular form as well, i.e. interface name ApplicationQuickAction
with the props from the mapped values.
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.
Hm ok, for compatibility reasons. I'll introduce a singular ApplicationQuickAction
interface and use that for the values in the ApplicationQuickActions
interface. Good enough for now.
const announceNavigationItems = ( | ||
applicationId: string, | ||
store: Store<unknown>, | ||
navigationItems: ClassicApplicationScript['navItems'] |
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 not
navigationItems: ClassicApplicationScript['navItems'] | |
navigationItems: ApplicationNavigationItem[] |
? 🤔
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.
Or in other words: you already introduced types, why not use them here? Instead you're creating a dependency to the ClassicApplicationScript
which should not be needed in the (more) generic runtime API.
const announceTranslations = ( | ||
supportedLanguages: { [key: string]: string }, | ||
translations: unknown, | ||
appTranslations: ClassicApplicationScript['translations'] |
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.
Same here, why reference the ClassicApplicationScript
? Would be nicer to introduce a dedicated translation type in types.ts
and then reuse it here.
*/ | ||
const announceQuickActions = ( | ||
store: Store<unknown>, | ||
quickActions: ClassicApplicationScript['quickActions'] |
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.
Same here, why reference the ClassicApplicationScript
?
412445d
to
90504cc
Compare
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.
Took the liberty of fixing some minor things in this PR. Failing hard with the whole system on one application not being able to load is a no-go - but since the next release is 2 weeks ahead we'll have enough time to fix that in a different PR. Fine to merge this now! Thanks for all the urgently needed cleanup! ❤️
Results for oCISFiles1 https://drone.owncloud.com/owncloud/web/19075/52/1
|
SonarCloud Quality Gate failed. |
Results for oC10SharingInternalUsersSharingIndicator https://drone.owncloud.com/owncloud/web/19079/28/1
|
Results for oC10Files1 https://drone.owncloud.com/owncloud/web/19079/12/1
|
The main reason for this pr was to provide a skeleton application which showcases how applications can inject file-handlers.
The challenge here was that in some cases the application needed to obtain those information from an api which can take time to process. It was not possible to wait for applications and the runtime did proceed even if the application hasn't finished loading what ever it needed (e.g. promises).
To tackle that case it was necessary to introduce such a logic (hooks). The runtime now understand 3 types of hooks.
initialize
this hook gets fired once the application is requested and got loaded by the runtime, it is not safe to call other applications from here because it's not guaranteed that all other applications have finished their registration process.
ready
all other applications are registered, this is a good place to call other applications or provide needed third party application data
mounted
this gets called after the main ui from the runtime got announced to the browser, from here every action will happen AFTER the ui is in place.
we took the opportunity while working on the runtime to refactor and demystify the whole runtime boot logic and rewrite it from the ground up in typescript.
the main reasons for doing this:
the application boot logic is now class-based and introduces a 'NextApplication' style, it takes care that all current applications still work by doing an automatic conversion between current and next. The next style applications can't be used at the moment, we first need to decide how the public interface could look like.
It also introduces a runtime API which only exposes (by DI) allowed functionality to the requested applications.
This can change in the future and for now we only have to decide how the public application runtime API should look like, everything else can be changed later without breaking existing applications.
This is a prerequisite for:
this is wip and the skeleton app needs more attention and functionality