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

Geppetto Redux refactoring #282

Open
wants to merge 18 commits into
base: development
Choose a base branch
from
Open

Geppetto Redux refactoring #282

wants to merge 18 commits into from

Conversation

ddelpiano
Copy link
Contributor

@ddelpiano ddelpiano commented Jun 9, 2020

Geppetto Redux refactoring - Page 1

Lucidchart exported here above

Walkthrough in the implementation:

StoreManager: This is the singleton, core part of the new implementation. It handles the StoreManager itself creation, the store creation and plug together all the part related to actions, middleware, layoutManager and the injectReducer that allows us to add reducers on the fly from the application side.

Actions: self esplicative.

Middleware: here we defined 2 interesting things, first the middleware itself, second the eventsCallback, an object that define a list where we can push functions for each action, it allows us to run callbacks linked to the action just triggered. This will be slightly modified to have 2 different lists, one pre-next(action and one post-next(action).

Reducers: self esplicative, they are then combined in the store.

Store: self esplicative.

reduxConnector: following the pattern suggested by @filippomc with the LayoutManager, I am now centralizing all the components that needs to be plugged to the redux store in a single point, this will allow us to maintain the same component name of the original react component and easily import all the stuff we need from a single source point.

LayoutManager: LayoutManager implemented by @filippomc , small change has been introduced here and in the WidgetFactory, this to allow the generic geppetto application to inject custom widgets that we will create on the application side and needs to be handled within the layout manager ( here and here ).

Still to do:

  1. merge splitRefactoring with this.
  2. fix tests.
  3. remove still some actions that have been ported from the geppetto events and can (probably) go away, even after discuss it I prefer to leave them here for now since I will have leftover code pending in other parts and I prefer to clean whatever related to these actions when done.

@ddelpiano ddelpiano marked this pull request as draft June 9, 2020 23:01
@ddelpiano ddelpiano self-assigned this Jun 9, 2020
@ddelpiano ddelpiano added this to In progress in Geppetto client roadmap via automation Jun 9, 2020
@ddelpiano ddelpiano linked an issue Jun 9, 2020 that may be closed by this pull request
@tarelli
Copy link
Member

tarelli commented Nov 17, 2020

@ddelpiano when we have time in the next weeks if we can bring in the latest from development then we can review and merge this

@tarelli
Copy link
Member

tarelli commented Jan 26, 2021

@ddelpiano when do you think we could do this so we can start the review? Don't worry, I know you are busy with a ton of things :)

Copy link
Contributor

@filippomc filippomc left a comment

Choose a reason for hiding this comment

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

@ddelpiano left some comments, not really requesting changes but better see if we can improve something and start some discussion before merging this huge work!

The main point I'd like to discuss is the elimination of GEPPETTO.trigger: it could be easily map with the new event system so reducing a lot of problems to legacy applications.

geppetto-client/js/common/GEPPETTO.CommandController.js Outdated Show resolved Hide resolved
geppetto-client/js/common/GEPPETTO.StoreManager.js Outdated Show resolved Hide resolved
geppetto-client/js/common/StoreManager.js Show resolved Hide resolved
@@ -0,0 +1,416 @@
/**
* Wraps the FlexLayout component in order to allow a declarative specification
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for me: need to merge with the latest version of LayoutManager that supports component state import/export

@@ -0,0 +1,54 @@
import * as React from 'react'

import { Widget } from './model';
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see the logic that allows to extend the WidgetFactory. Maybe this is outdated or used as a base class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure about this, I copied all available for the layout manager when we discussed about that, not sure if that code portion moved forward but in case I think we should address that in a separate card.

Copy link
Contributor

Choose a reason for hiding this comment

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

This feature was not implemented in my version of the WidgetFactory because it was directly implemented in the application. Once it becomes part of the library we need a way to extend the functionality of the WidgetFactory to allow the application to add custom components to the layout.

geppetto-client/js/common/middleware/geppettoMiddleware.js Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ Manager.prototype = {
window.Project.persisted = true;
window.Project.readOnly = false;

GEPPETTO.trigger(GEPPETTO.Events.Project_persisted);
GEPPETTO.StoreManager.actionsHandler[GEPPETTO.StoreManager.clientActions.PROJECT_PERSISTED]();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to do all the replacements but at the same time I'd continue to support GEPPETTO.trigger. All applications are using it and the mapping with StoreManager calls is straighforward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

support for geppetto trigger is trivial and I pushed it in the current branch, having support for the old on/off methods is not rocket science but it will take me a little bit of time since not planned originally.
Since the backward compatibility emerged just now, do we want to spend additional time in creating wrappers for GEPPETTO.on/off to mock whatever functionality was there before or do we want to put back backbone and continue to use that in parallel with redux?
The on/off mechanism will require the new eventsCallback logic to be plugged into the wrapper to take into account of the context and eventually extra variables passed to the parameter.
Right now the eventsCallback take in input only the action with eventually properties from this object that we want to have in the callback, I will have to figure out how to plug that logic in the wrapper if that is the solution that we want to follow.

@ddelpiano ddelpiano marked this pull request as ready for review April 23, 2021 15:44
@ddelpiano ddelpiano changed the base branch from development to master April 23, 2021 15:44
@ddelpiano ddelpiano changed the base branch from master to development April 23, 2021 15:44
@ddelpiano
Copy link
Contributor Author

@lrebscher latest changes have been pushed as per PR review requested by @filippomc.
Fyi, a reminder that this refactoring has still an issue with the plot components, as discussed briefly with Filippo I think the problem is related to the fact that before the time series were updated triggering the same event hundreds of times in a short amount of time where now the redux reducer I think it's merging all these duplicated calls at into a single one and that is why currently I can see the time series updated only at the end.
Previously the plan was to merge the latest development into this pr, go forward with the review and then decide what to do with this issue, we need to decide if this issue will be handled inside the PR or it will a separated issue.
Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Geppetto client roadmap
  
In progress
Development

Successfully merging this pull request may close these issues.

Replace the events handled by backbone with redux
3 participants