[GitHub] [incubator-devlake] e2corporation commented on issue #2744: [refactor][config-ui] Some development refactor

[GitBox <gi...@apache.org>]

e2corporation commented on issue #2744:
URL: https://github.com/apache/incubator-devlake/issues/2744#issuecomment-1215829568

   Hello @likyh Thanks for proposing some refactoring changes on UI. I've noted my responses to your feedback points below. As we briefly discussed earlier, transformation logic has already been refactored and optimized in an upcoming Pull Request for Blueprint Settings (#2659). Any proposed refactoring will have to be accommodated after the `V0.13.0 Milestone` features for UI have been completed. You will need to familiarize yourself with the new changes after Blueprint Settings ticket has been merged to main.
   
   TypeScript would be a great addition at later stage, It's already entered my mind a few times. Usually a decision to adopt typescript is made at the beginning of a project. While it's doable, refactoring the whole ui codebase to typescript directly on the heels of an upcoming release is not a wise decision. Before TypeScript is added, use of formal UI Data Models will resolve most concerns you have with type safety.
   
   > only update the local state (defined in the current component by useState) in the function of useEffect, which depends on the props (state passed by the parent).
   
   I'm not sure if I understand this statement or if it fully explains what you are trying to say. It sounds like you are saying that state should solely be updated inside an effect, which is not entirely accurate or true and would lead to many circular loops if that were the case. Many state updates are already formally handled within Effects, though effects can update state or fire a dependent callback that also manipulates state. In some scenarios where a formal handler isn't needed or in lower level child components, state is modified directly within the `onClick` handlers where needed so as to reduce the need for creating yet another handler that must be wrapped in a callback.
   
   > call the function onXXX/handleXXX/setXXX passed by the parent only when listening on the local state bound with UI.
   1 and 2 mean we cannot use useEffect to update the parent state by depending on another parent state like:
   https://github.com/apache/incubator-devlake/blob/main/config-ui/src/pages/configure/settings/jira.jsx#L99-L105
   
   Not sure if I understand what problem you are trying to identify here, initial state is passed down from parent. In the case of JIRA (like all other providers) all it's settings is routed through one settings handler (`onSettingsChange`) which stores transformation settings keyed by Board ID (JIRA) and Project Name (GitHub) and Project ID (GitLab)
   
   > use useMemo when a var is computed and has no side effects when updated. This action has been used in activeTransformation, and we need to use it in all computed vars. It's an excellent way to help the developer know how a state affects another state.
   
   It's fine to propose additional memoization calls to further optimize, state variables that are relying on other state variables for current value may be the first place to start. `useMemo` is already being implemented like you mention. Feel free to define and extract additional memoized values to a higher level such as a an existing hook if necessary. You can also implement new logic with `useReducer` Hook which maybe leveraged to reduce extra state variables.
   
   > pack the logic of one data model in a hook and define some UI state in the appropriate component instead of global.
       https://github.com/apache/incubator-devlake/pull/2552/files#diff-6e1e4ac7737de3bc6a223b9020b4b7012f28750afc3c12a98517e1628f214ec4R80
       The logic written in the hook will be more readable than written in some different files or callbacks. And the data hook can avoid related updating/query logic in the different callback.
   
   I've created a Data Scopes Manager hook with my PR #2659 (Blueprint Settings Management). Any logic for maintaining transformations and the associated state data properties have been migrated here. Your concerns with "newTransformation" have already been resolved with the v0.12.0 release actually, though the variable signature was kept it is not being used. That variable has since been removed with upcoming changes in `v0.13.0`. There is only 1 active transformation property defined and it's managed by the DSM Hook mentioned above.
   
   > These ways can help us reduce the number of useState and useEffect. But special cases always exist. If we have to pass state and callback from pageComponent to detailComponent, let's use Typescript to clarify how to use them.
       An example:
       https://github.com/apache/incubator-devlake/blob/main/config-ui/src/pages/blueprints/create-blueprint.jsx#L591-L594
       addProjectTransformation call setConfiguredProject in create-blueprint.jsx;
       addProjectTransformation passed to DataTransformations.jsx;
       and it passed to StandardStackedList.jsx as onAdd and onChange;
   
   https://github.com/apache/incubator-devlake/blob/main/config-ui/src/pages/blueprints/create-blueprint.jsx#L596-L599
   addBoardTransformation and setConfiguredBoard has the similar path.
   
   But notice, they have similar names and similar using, but the param of setConfiguredProject is ID and the param of setConfiguredBoard is Object ...
   
   To directly answer your concern it's not ideal that one is a string and the other is an object, but once you are aware of that knowledge (which you easily discovered) it's not a huge problem either, nor that confusing. It's _consisent_ this way everywhere `configuredProject` is used directly as value `configuredBoard` is an Object that in most cases uses it's id as an identifier. This is not so life changing that we need TypeScript to solve. This problem can easily be addressed with the use of a formal Data Model (as I mentioned earlier there will be Data models added to UI so we can add formality to all the user controlled values), which will then upgrade the user controlled project values to _Objects_, much like jira boards already are.
   
   I think you first should understand that this behavior is not purely intentional but natural side effect of the Input components at play that drive this data, and also that Project Names & IDs are user-controlled values where as JIRA Boards are driven from pre-built API Data, typing in these components simply filter/search for data. Currently, GitHub Projects and Gitlab Projects use a **Multi-tag Input component** where a user defines/enters the values and they are processed as strings. 
   
   JIRA Boards on the other hand use a **MultiSelect Tag** Input component that is driven from API data, and the list items are required to be objects in the first place. Now I could have opted to just store the ID values for JIRA operations (ie selecting the board), but I wanted to store the whole object instead so I could have access to Board Name and other context data preserved.
   
   Once Data Models are added we can formally construct differing project types with these models so it's not only clear that all configured entities will be objects, but their structures would also be well defined.
   
   In regards to `onAdd` and `onChange` receving the same handler this is intentional. From the child component's perspective it has these trait actions, `onAdd` and `onChange`. It just so happens at the implementing level and the needs of the feature they both share the same task (to define the current board) which is why these properties received the `addBoardTransform` handler action, which wraps around setting the current board into state.
   
   ```
   ### Example UI Modeling for Illustration 
   
   The effects and onClick handlers and parent handlers (Components, Hooks etc) responsible for setting these values will be upgraded to wrap the user controlled values in a new model definition.
   
   # JIRA
   const jiraProject = new BoardProject({boardId: 1})
   // We could also create a provider specific model and call it `JIRAProject` or `JIRABoard`. If we create a generic model like `BoardProject` however we can re-use it for another Provider that gets added that also requires the concept of "Board" to be defined.
   
   # GitHub
   const gitHubProject = new RepositoryProject({owner: 'e2corporation', repositoryName: 'incubator-devlake', type: 'repository'})
   
   # GitLab
   const gitLabProject = new RepositoryProject({projectId: 20381029, type: 'numeric'})
   
   ```
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org