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: Centralize error handling #3792

Merged
merged 13 commits into from
Dec 16, 2024
Merged

Conversation

grebaldi
Copy link
Contributor

@grebaldi grebaldi commented May 30, 2024

refs: #3119
prelude to: #3773

The @neos-project/neos-ui-error package had been introduced in #3759. During the discussion of that PR, @mhsdesign suggested it would be a good idea to centralize other error-handling mechanisms in that package (namely the high-level ErrorBoundary, see: #3759 (comment)).

As I was working on #3773, I realized that I would need to decouple FlashMessages from the redux store and expose a different mechanism to use them. So I decided to take the centralization step first.

Bottom-line, this PR moves the following concepts into @neos-project/neos-ui-error:

The @neos-project/neos-ui-error package now exposes a showFlashMessage function that can be used to display FlashMessages from anywhere in the system. The state of FlashMessages is now decoupled from the redux store. It is handled in a global, singleton Observable instead.

To achieve this, I introduced a new state handling mechanism via the @neos-project/framework-observable package using the approach I had outlined in #3331.

Thus far, I have implemented the createObservable and createState mechnisms with additional tests and documentation. Later on, this package will allow us to replace redux opportunistically where needed.

This change is breaking, because I have removed the now obsolete parts of the redux store that were related to flash messages. It is unlikely that third parties are relying on the parts I've removed, since they are implementation details related to the read-side (So unless a plugin needs to know what flash messages are currently displayed - which I doubt -, this should be safe).

The only thing still intact is the UI.FlashMessages.add action, because plugins may actually use it and currently have no other way to display flash messages. I have implemented a saga to redirect those action calls to the new mechanism.

My plan is to expose showFlashMessage publicly via a global JS API, that I will introduce in #3773. In that PR I will then deprecate the intermediate saga and the UI.FlashMessages.add action.

@github-actions github-actions bot added the 9.0 label May 30, 2024
@grebaldi grebaldi force-pushed the task/centralize-error-handling branch from 092d97d to 9eed050 Compare June 11, 2024 09:18
grebaldi added 5 commits June 13, 2024 12:29
This introduces a very basic implementation of the observable pattern
for the Neos UI.

This will allow us to replace redux opportunistically in various places.
This new package contains react bindings for the other recently
introduced package `@neos-project/framework-observable`.
...by using the newly introduced observable primitives. The
neos-ui-error package now exposes a method to show flash messages.

A saga has been created to redirect redux actions (that may still be
used in plugins) to that method.
@grebaldi grebaldi force-pushed the task/centralize-error-handling branch from 9eed050 to 2caf38a Compare June 13, 2024 10:30
@grebaldi grebaldi changed the title WIP: Centralize error handling !!!FEATURE: Centralize error handling Jun 13, 2024
@grebaldi grebaldi marked this pull request as ready for review June 13, 2024 11:08
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.

Well done ❤️ especially thanks for keeping actionTypes.UI.FlashMessages.ADD in tact as we promote this as part of a guide "how to use neos ui redux" ^^ https://docs.neos.io/guide/advanced/extending-the-user-interface/react-extensibility-api#sagas


if (!isClosing.current) {
isClosing.current = true;
setTimeout(() => onClose(id), 100);
Copy link
Member

Choose a reason for hiding this comment

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

i do not quite grasp this timeout here and also was wondering if its ideal that we delegate this timeout logic to this nested component instead of handling it in a higher level ... but please dont see this as critique but rather me trying to learn the tricks ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've abstained from refactoring this stuff. This is only copied over from the original implementation and I agree that it's a bit smelly, but I didn't want to do more than strictly necessary to achieve the architectural goals.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect i oversaw that. In that case im even more grateful that its just moved as this should simplify the review :)

@@ -0,0 +1,152 @@
# @neos-project/framework-observable
Copy link
Member

Choose a reason for hiding this comment

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

thank you for providing extensive documentation for these framework parts.

I think we already partially discussed the idea of the "The Client-side framework" from #3331:

@neos-project/framework-observable

The @neos-project/framework-observable package provides a very simple implementation of the Observable-pattern (see: https://github.com/tc39/proposal-observable).

Furthermore, based on Observables, it establishes two key primitives for use in the UI:

State

A State observable represents a value that changes over time. It can be asked about its current value and any subscriber will receive the current value immediately.

It also provides an update method, that allows to push the next value calculated from the current value.

Channel

A Channel observable represents a sort of message bus. A subscriber will receive all messages that have been published to the Channel since the subscription was established.

It provides a publish method to push messages onto the channel.

@neos-project/framework-react-hooks

The @neos-project/framework-react-hooks package contains a bunch of essential React hooks to allow binding state to React components.

Most notably it contains the useLatestValueFrom hook, that allows to bind Observables to components.

And for the DTO serialisation and de-serialisation (@neos-project/framework-schema) you elaborated why it makes sense to provide a custom implementation instead of using a npm library: #3331 (comment)

Regarding observers and state handling i remember having talked to you regarding using rx-js vs a custom implementation like @neos-project/framework-observable and i think the reasons for a custom implementation were customisability (limited feature scope we only need to implement what we need), smaller bundle size, and that Observable will hopefully come to native ECMA script which we definitely want to profit from if available and this can be ensured by having it all under control.
But im a little fuzzy on the details and dont quite remember it all. So it would be great to discuss this to fill in the gaps. In the end this adds complexity to our codebase and thus the benefits and downsides should be carefully considered. When we decide for this library an outlined discussion would also help future maintainers to understand our decision ;)

EDIT:
I just skimmed over the implementation and its a super slim layer so +1 from my side ;) And we could consider this dicussion closed ^^ The documentation and tests are more than the code :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've summarized it perfectly :)

@markusguenther
Copy link
Member

This PR is now in sync with the latest 9.0 changes. Thanks for all the work @grebaldi ❤️

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

LGTM

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.

As discussed with christian and markus we like this change very much thanks :)

We have to keep in mind that moving back from redux of course means that the store is not easily debuggable via browser extension and that plugins cannot hook into the process. This means we likely end up with more explicit defined extension points instead of one big store. But for the ui is this exactly what wee need. Small components like flashmessage especially profit from that as there is absolutely no need for extensibiliity.

@mhsdesign mhsdesign merged commit b22d46b into 9.0 Dec 16, 2024
10 checks passed
@mhsdesign mhsdesign deleted the task/centralize-error-handling branch December 16, 2024 10:06
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