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: Reform foundation and boot process of the UI #3682

Merged
merged 27 commits into from
Feb 2, 2024

Conversation

grebaldi
Copy link
Contributor

@grebaldi grebaldi commented Jan 10, 2024

refs: #3119
next: #3773

This is the first in a series of PRs that will enable the creation of multiple applications based on a Neos UI core.

This PR turns the Backend/Index.html Fluid template into a dedicated ApplicationView. This ApplicationView has the sole purpose of building the foundational HTML document (via string concatenation) for bootstrapping the Neos UI.

Formerly, Fusion was used to retrieve and render the initial data for the UI. This task has been delegated to a set of dedicated classes, namely:

  • ConfigurationProvider - retrieves the nodeTree and structureTree segments from Neos.Neos.userInterface.navigateComponent settings, allowedTargetWorkspaces from the ContentRepository's WorkspaceService as well as the nodeTypeSchema and translations endpoints.
  • RoutesProviderInterface - retrieves all other routes/endpoints required for communication with the Neos server application
  • FrontendConfigurationProvider - reads and preprocesses the Neos.Neos.Ui.frontendConfiguration settings that are mostly used by third-party plugins to share data between server and client
  • NodeTypeGroupsAndRolesProviderInterface - retrieves information about node type roles and node type groups. Roles are a dedicated UI-concept that is meant to distinguish between document, content and collection nodes. Groups refer the grouping of node types in the creation dialog.
  • MenuProvider - retrieves all data needed to render the main burger menu located in the top left corner of the UI.
  • InitialStateProvider - reads and preprocesses the Neos.Neos.Ui.initialState settings that are used to hydrate the UI's redux store

The responsibilities of these classes is entirely derived from their former Fusion counterparts. All of them have been marked @internal to allow for later removal. Nonetheless, it remains possible to implement their accompanying interfaces and replace their implementations via Objects.yaml. This enables rare edge cases and unplanned extensibility.

The splash screen, formerly a Fluid template, is now hard-coded into the ApplicationView class, which renders the Neos.Neos.Ui.splashScreen setting obsolete. It has therefore been removed.

Furthermore, this PR reforms the client-side boot process of the UI. This includes the following changes:

  • Scripts (The UI main script, as well as plugins) are now loaded in the <head/> with defer
    • This way, the UI does not need to listen to DOMContentLoaded in order to discover the #appContainer element. This allows a lot of the UI boot process to become synchronous.
  • The main boot function is no longer a Redux Saga. It is instead an asynchronous function.
    • Also, the various boot-related sub-processes (initializing plugins, loading translations, etc...) have been wrapped in dedicated functions.
  • The initial data for the boot process is now JSON.parsed instead of window['_NEOS_UI_*']ed
    • JSON.parse is somewhat more performant, because for JSON only a subset of ECMAScript needs to be parsed. (Though, this is not all that significant given the amount of data, but still 🙂)
  • The data-first-tab attribute has been removed from #appContainer
    • I was unable to find any references to it within the code base, so I removed it, because I didn't know what exactly to put in there. I took it for a historical leftover. Please let me know if I'm mistaken :)

Remaining TODOs

  • Replace Fusion with a dedicated application view class
  • Use <head>-scripts + defer instead of <body>-scripts for script inclusion
  • [-] Turn the remaining Fluid templates into Fusion prototypes as well (optional)
    • [-] Backend/ConfigurationVersion.html
    • [-] Backend/Guest.html
    • [-] Backend/GuestNotificationScript.html
    • [-] Error/ErrorMessage.html
    • EDIT: Scope is large enough as it is - these can be done later
  • Remove the actions.System.boot() dispatch?
    • Noone seems to listen to it...
  • Remove Neos.Neos.Ui.splashScreen configuration
    • Relies on fluid partial that no longer exists
    • Replacement? -> EDIT: None. It's hard-coded now. We can open this up later if there's demand for it.
  • [-] Remove fusion objects that are no longer relevant
    • [-] Neos.Neos.Ui:RenderConfiguration
    • EDIT: Nope, this is still being used for the guest frame
  • [-] Deprecate EEL-Helpers that are no longer relevant
    • [-] Neos.Ui.Workspace
    • EDIT: Nope, this is still being used in settings
  • Add doc comment that explains node type roles and node type groups concepts
  • Rename NodeTypesProviderInterface -> NodeTypeGroupsAndRolesProviderInterface
  • Use an interface to className mapping in Objects.yaml instead of explicitly configuring BackendController properties
  • Prevent duplication of defer attribute on <script/>-includes through Neos.Neos.Ui.resources settings

Note to fellow maintainers

In order to preserve the usefulness of the commits in this PR, please do not merge any of the version branches into this branch. To update the code with the latest changes in 9.0, please do an interactive rebase instead:

git fetch upstream
git checkout -b feature/multi-app/01-foundation upstream/feature/multi-app/01-foundation
git rebase -i upstream/9.0 feature/multi-app/01-foundation

This will require

git push -f upstream feature/multi-app/01-foundation

afterwards.

This way, every commit will be replayed on top of 9.0, and can be adjusted seperately.

@github-actions github-actions bot added the 9.0 label Jan 10, 2024
@grebaldi grebaldi force-pushed the feature/multi-app/01-foundation branch from 330a48d to 5922ea3 Compare January 10, 2024 21:27
@grebaldi grebaldi changed the title WIP: Reform foundation and bootprocess of the UI WIP: Reform foundation and boot process of the UI Jan 10, 2024
@grebaldi grebaldi marked this pull request as draft January 10, 2024 21:30
@grebaldi grebaldi force-pushed the feature/multi-app/01-foundation branch from 5922ea3 to b1dbfd4 Compare January 12, 2024 20:00
@mhsdesign
Copy link
Member

mhsdesign commented Jan 13, 2024

Thank you so much!!! This really cleans up many things that are needlessly messy and hard to understand. I added already a dec comments to your text and will look over the code later ;)

Remove the actions.System.boot() dispatch?

I only know that I have used the init see:
#3098 (comment)
(And just noticed the issue might be generally interesting for you;))

About:

Remove Neos.Neos.Ui.splashScreen configuration

and

The data-first-tab attribute has been removed from #appContainer

I have never heard of those things so I assume they can go;)

are now loaded in instead of

Today I actually prefer the use of the defer attribute … feels cleaner to have them in the head … but it’s really not important


Note to myself:
the commit is probably the one which will get outdated the easiest and has to be taken care of carefully.

TASK: Group all initial data variables into a single view variable

Also i think we should incorporate #2920 into here.


Thanks again!!!! :D

@grebaldi
Copy link
Contributor Author

Hi @mhsdesign,

thanks a lot for your very valuable feedback 👍

Today I actually prefer the use of the defer attribute … feels cleaner to have them in the head … but it’s really not important

I'm always afraid of introducing race conditions when it comes to this stuff 😅. Well, MDN states:

This Boolean attribute is set to indicate to a browser that the script is meant to be executed after the document has been parsed, but before firing DOMContentLoaded.
(see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#defer)

So, I guess this means we have a guarantee that the script can see the whole document? I mean document has been parsed sounds like that, but before firing DOMContentLoaded strikes me the wrong way - so as if we're at a point of execution at which the state of the DOM isn't exactly determined.

I'm with you, that we should use defer, especially if that means that the browser's rendering thread doesn't have to block for parsing our UI bundle before displaying anything. So, if you can take my above stated fear from me, I'll gladly change the script integration to defer :D

I also have some ideas bouncing around in my head with regards to the code issues you've pointed out. I similarly feel that the fusion foundation as of right now is less than ideal. I see two possible way out of here:

A) Build dedicated Fusion @class implementations for resource collection

So, basically, this would mean to back the following prototypes with classes:

  • Neos.Neos.Ui:Component.InitialData
  • Neos.Neos.Ui:Component.HeadStylesheets
  • Neos.Neos.Ui:Component.HeadIcons
  • Neos.Neos.Ui:Component.Scripts

This would move all the resource-collecting logic out of the controller and out of the fusion code. It'd also eliminate the need for include: resource://Neos.Fusion/Private/Fusion/Root.fusion (at least I think it would :)).

B) Replace Fusion with a dedicated application view class

Because: what's the point of using Fusion to initialize the UI anyway?

A dedicated view could take care of hiding the resource-collecting logic from the controller. The initialData contents would map nicely onto view variables. The view could just concatenate the HTML document - nice, short and fast 🙂

wdyt?

@grebaldi
Copy link
Contributor Author

Oh, and regarding #3098:

I don't think that this issue would be fixed as of right now. But I'm going to try to move around some bits of the boot process (should be possible), so plugins have an easier time reading this stuff.

@mhsdesign
Copy link
Member

I actually dont know that much about defer - just used it - so lets stick at first with js body scripts ^^

B) Replace Fusion with a dedicated application view class
Because: what's the point of using Fusion to initialize the UI anyway?

And yeah i thought so too... well lets just use b? :D
The only thing would be of course unplanned extensibility, but i havent used that and everything useful is already available as api via settings.

especially things like

# TODO: dirty hack to not have to re-implement neos:backend.configurationCacheVersion VH
version = Neos.Fusion:Template {
    templatePath = 'resource://Neos.Neos.Ui/Private/Templates/Backend/ConfigurationVersion.html'
    @process.trim = ${String.trim(value)}
}

make me rethink if fusion is the right choice 😅

Aaand we might finally remove this weirdly namespaced Neos.Ui.PositionalArraySorter.sort and Neos.Ui.Api helpers 😂

@grebaldi
Copy link
Contributor Author

@mhsdesign

I actually dont know that much about defer - just used it - so lets stick at first with js body scripts

I looked into the spec, but it is just as imprecise about the exact timing as MDN is:

If the async attribute is not present but the defer attribute is present, then the classic script will be fetched in parallel and evaluated when the page has finished parsing.

(see: https://html.spec.whatwg.org/multipage/scripting.html#attr-script-defer)

Then I found this article that is very confident in saying:

Whereas DEFER scripts don’t execute until the HTML document is done being parsed (AKA, DOM Interactive or performance.timing.domInteractive).

(see: https://calendar.perfplanet.com/2016/prefer-defer-over-async/#attributes)

Sadly no sources to back that up...

I guess it wouldn't make sense for any defer implementation to not have the DOM available after document parsing. So, I think we can relatively safely assume that <head>-scripts + defer will work fine. I'll add that to the list :)

The only thing would be of course unplanned extensibility

Actually, you'd have to do quite some acrobatics to hook into the backend fusion view. It cannot be extended via regular ol' autoInclude - which was by design, because this view is not supposed to be interfered with.

I think the reasoning for using fusion back then was that it provided a unified high-level API for both MVC/Routing and ContentRepository, which are both major concerns for initial hydration of the UI. But this was always supposed to be a strictly hidden implementation detail.

[...] everything useful is already available as api via settings.

#################################
# INTERNAL CONFIG (no API)
#################################

☝️ Well... Actually those settings are not supposed to be API. However, this is certainly not the best way to hide stuff 😅

I'd like to get rid of this mechanism as well, but that exceeds the scope of this PR.

well lets just use b?

I'll add that to the list as well :)

@grebaldi grebaldi force-pushed the feature/multi-app/01-foundation branch from c5216a0 to 9d58de7 Compare January 16, 2024 14:55
@mhsdesign
Copy link
Member

mhsdesign commented Jan 16, 2024

Thanks for the housekeeping and implementing the defer :D

Actually, you'd have to do quite some acrobatics to hook into the backend fusion view. It cannot be extended via regular ol' autoInclude - which was by design, because this view is not supposed to be interfered with.

yes i fully agree and also talked to @Sebobo about this. There is absolutely no need to have it in fusion and php would be a better fit.

But to keep this pr rather simpler, i would say that we could keep the fusion code in one messy file for now and refactor it to php with another followup pr?

I especially dont want to merge the include: resource://Neos.Fusion/Private/Fusion/Root.fusion part :D

But if you think its doable in scope of this pr i wouldnt mind moving to php quicker.

@grebaldi
Copy link
Contributor Author

But if you think its doable in scope of this pr i wouldnt mind moving to php quicker.

Already on it :D

@grebaldi grebaldi force-pushed the feature/multi-app/01-foundation branch 2 times, most recently from d288e5f to dc750be Compare January 22, 2024 12:52
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

WOW ❤️ 🎉 thank you so much for so cleanly moving the neos ui integration to php.
The code looks way more obvious and is easier to follow! Thanks a lot!

Classes/Domain/NodeTypesProviderInterface.php Outdated Show resolved Hide resolved
Configuration/Objects.yaml Outdated Show resolved Hide resolved
@mhsdesign
Copy link
Member

Fyi this will be a bit nasty to rebase to #3689 but i think doable, so i would propose to merge that first?

@grebaldi
Copy link
Contributor Author

@mhsdesign

Fyi this will be a bit nasty to rebase to #3689 but i think doable, so i would propose to merge that first?

#3689 should definitely go first. I don't think rebase will be that big a deal :)

This way, the UI does not need to listen to `DOMContentLoaded` in order
to discover the `#appContainer` element. This allows a lot of the UI
boot process to become synchronous.
Functionally, it stays the same, but the boot process is no longer a
Redux Saga. It is instead an asynchronous function that runs when the
`DOMContentLoaded` event was fired.

Also, the various boot-related sub-processes (initializing plugins,
loading translations, etc...) have been wrapped in dedicated functions.
...rather than inlineing it. `JSON.parse` is somewhat more efficient,
because for JSON only a subset of ECMAScript has to be parsed.
The error message has been adjusted to the look of the global
ErrorBoundary, re-using its stylesheet.
grebaldi and others added 6 commits January 22, 2024 16:47
The fusion path is now dynamically determined by controller / action
This enables additional actions with separate templates for that controller.
…apping the UI

The `ApplicationView` builds the foundational HTML document (via string
concatenation) for bootstrapping the Neos UI.

Formerly, Fusion was used to retrieve and render the initial data for
the UI. This task has been delegated to a set of dedicated classes,
namely:

- `ConfigurationProvider` - retrieves the `nodeTree` and `structureTree`
segments from `Neos.Neos.userInterface.navigateComponent` settings,
`allowedTargetWorkspaces` from the ContentRepository's
`WorkspaceService` as well as the `nodeTypeSchema` and `translations`
endpoints.
- `RoutesProviderInterface` - retrieves all other routes/endpoints
required for communication with the Neos server application
- `FrontendConfigurationProvider` - reads and preprocesses the
`Neos.Neos.Ui.frontendConfiguration` settings that are mostly used by
third-party plugins to share data between server and client
- `NodeTypeGroupsAndRolesProviderInterface` - retrieves information about
node type roles and node type groups. Roles are a dedicated UI-concept that
is meant to distinguish between document, content and collection nodes.
Groups refer the grouping of node types in the creation dialog.
- `MenuProvider` - retrieves all data needed to render the main burger
menu located in the top left corner of the UI.
- `InitialStateProvider` - reads and preprocesses the
`Neos.Neos.Ui.initialState` settings that are used to hydrate
the UI's redux store

The responsibilities of these classes is entirely derived from their
former Fusion counterparts. All of them have been marked `@internal` to
allow for later removal. Nonetheless, it remains possible to implement
their accompanying interfaces and replace their implementations via
`Objects.yaml`. This enables rare edge cases and unplanned
extensibility.

The splash screen, formerly a Fluid template, is now hard-coded into the
`ApplicationView` class, which renders the `Neos.Neos.Ui.splashScreen`
setting obsolete. It has therefore been removed.
With the new `ApplicationView` this code is no longer in use anywhere.
This action used to be fired before all asynchronous operations at the
beginning of the boot process.

Since there's not a single subscriber within the UI code base that
listens to that action, it has now been removed.
@grebaldi grebaldi force-pushed the feature/multi-app/01-foundation branch from 0fc5ed9 to e092605 Compare January 22, 2024 16:02
This also removes handling of the legacy first level 'defer' setting in
the `Neos.Neos.Ui.resources` settings. If it is set, it will be ignored.
@grebaldi grebaldi changed the title WIP: Reform foundation and boot process of the UI !!!FEATURE: Reform foundation and boot process of the UI Jan 22, 2024
@grebaldi grebaldi marked this pull request as ready for review January 22, 2024 16:43
@mhsdesign
Copy link
Member

mhsdesign commented Jan 22, 2024

Id say this is nearly ready to be merged. Because im a little paranoid i actually diffed the initial bootstrap html before and after and found a few differences worth noting.

First it seems that the User Settings module link was not included previously, but now shows up and will be rendered in the menu:

        {
            "label": "Neos.Neos:Modules:user.label",
            "icon": "fas fa-users",
            "children": [
                {
                    "icon": "fas fa-user",
                    "label": "Neos.Neos:Modules:userSettings.label",
                    "uri": "http:\/\/localhost:8081\/neos\/user\/usersettings",
                }
            ]
        }

There seems to be a module.hideInMenu or submodule.hideInMenu flag to toggle this.

image

And second, it seems that we boot the ui with more data: I found that everything mapped to the legacy globals but there is a new entry settings with seems to be an complete dump of the whole Neos.Neos.Ui configuration, at least by looking at its keys:

  • 'resources'
  • 'contentCanvas'
  • 'frontendConfiguration'
  • 'frontendDevelopmentMode'
  • 'nodeTypeRoles'
  • 'configurationDefaultEelContext'
  • 'documentNodeInformation'
  • 'initialState'
  • 'changes'
  • 'outOfBandRendering'

Edit. I found the cause. We json_encode all the $this->variables, but it was once thought be a good feature to have the settings automagically available see neos/flow-development-collection@b944521.
So i guess we need to namespace all the variables in something like booting or value like the json view idk.

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

I adjusted the previously mentioned parts a little and now everything is like expected.
I would prefer another or two reviews though ^^
Thanks again for the hard work!

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

LGTM, just left one question regarding the necessity of the Views.yaml

Configuration/Views.yaml Outdated Show resolved Hide resolved
@mhsdesign mhsdesign merged commit 1575ec0 into 9.0 Feb 2, 2024
10 checks passed
@mhsdesign mhsdesign deleted the feature/multi-app/01-foundation branch February 2, 2024 14:56
@mhsdesign mhsdesign restored the feature/multi-app/01-foundation branch February 2, 2024 14:57
@mhsdesign mhsdesign deleted the feature/multi-app/01-foundation branch February 24, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants