Reactjs todo dv new #14

jon-oblander · 2024-10-29T17:45:27Z

No description provided.

cerebrl

So far, this is looking pretty good. I left some comments, but will zero-in on the davinci-flow.js file as that's a really important component to get right.

cerebrl · 2024-10-29T21:19:37Z

reactjs-todo-dv/client/components/davinci-client/flow-button.js

+    renderForm(node);
+  };
+
+  return (


May I ask why the spinner component was removed?

i was thinking the flow buttons might actually be links at some point but it can be added back. im still trying to get my head around how the sdk behaves vs what the flow is built like in DaVinci. This OOTB flow uses buttons.

I'm not super educated on DaVinci flows, but with my understanding, I wouldn't think these "flow buttons" would ever be links as they don't result in a navigation (a change in URL), but an internal change of reference of the form to render.

Were you thinking that click of a flow button would result in a navigation change to a new page/view for rendering the new flow? Or, that the internal form itself just renders different form elements?

I was looking at the example app in javascript sdk repo and it was using links rather than buttons, that was the only reason.

Oh, sorry for the confusion. I may have called them links, but technically, they are buttons. The are links in just the visual sense: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/e2e/davinci-app/components/flow-link.ts#L12.

cerebrl · 2024-10-29T21:25:05Z

reactjs-todo-dv/client/router.js

+  return (
+    <BrowserRouter>
+      <Routes>
+        <Route path="login" element={<Login />} />


I noticed that the register path was removed from the routing. Can we preserve this feature, and have /register go straight to a register flow, rather than login and then having to click on register again?

technically we could go onto the next node using the sdk if the user clicked on the register button but that requires adding code based on a specific flow and impacting re-usability. It would be best to have separate flows, 1 for login, 1 for registration imo but something we could consider building into the example.

I'd suggest that if the "sign up" (register) link is clicked, we explicitly call the register flow through acr_values. That way, we immediately load up the flow that the user selected. Thoughts?

There is only 1 flow in the current OOTB DaVinci environments that encompasses both login and register. Might be easier to talk about this one on a call.

Okay, if it's more complicated, we can set this aside when we both have a moment to chat. So, don't focus on this atm.

cerebrl · 2024-10-29T21:26:13Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

I want to take a bit more time reviewing this file in particular. This is probably the most important and most difficult to write, so I'd like to make sure we get it as good as we can.

100% ... this first pass was based on examples and limited exposure to the sdk which is not feature complete yet. plenty of room to iterate for sure.

cerebrl

Okay, this is a review of just the davinci-flow.js file. There's a lot of suggestions here, and if any are confusing or not articulated well, just ping us in one of our channels. Ryan or I can help out.

As a reference, I'd like to suggest a closer look at our form.js file seen here: https://github.com/ForgeRock/sdk-sample-apps/blob/main/reactjs-todo/client/components/journey/form.js. It's by no means perfect, but many of the stated principles here are mostly implemented in that file.

Thanks!

cerebrl · 2024-11-01T03:00:20Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+ * @function DaVinciFlow - React view for a DaVinci flow
+ * @returns {Object} - React component object
+ */
+export default function DaVinciFlow({ config }) {


From a component level, here are the principles that our team follows when it comes to React:

State, both global and local, should be pure data, and not contain rendering elements/components

Rendering is the output of state change; the old f(x) Functional Principle

State logic should be encapsulated in a React hook

React components contain only rendering logic (if state value is X render Y)

React's ref should be avoided, if possible; limit `ref' to "referencing" live DOM elements

cerebrl · 2024-11-01T03:04:29Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+  const [components, setComponents] = useState([]);
+  const [pageHeader, setPageHeader] = useState('');


I'd recommend that these are removed from "state" and are just conditional rendered. This relates to #1 from the summary. I usually reserve using useState for managing asynchronous state that needs to trigger a component rerender.

cerebrl · 2024-11-01T03:05:43Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+
+  let client = useRef(null);
+
+  const renderComplete = async (successNode) => {


Let's remove the word "render" from functions as they don't technically render anything.

cerebrl · 2024-11-01T03:07:47Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+    const code = successNode.client?.authorization?.code || '';
+    const state = successNode.client?.authorization?.state || '';
+    await TokenManager.getTokens({ query: { code, state } });
+    const user = await UserManager.getCurrentUser();
+    methods.setUser(user.preferred_username);
+    methods.setEmail(user.email);
+    methods.setAuthentication(true);
+    navigate('/');


I think all of this can stay in a function, but I'd place this within a useEffect function and only have it run of we've reached success from a DaVinci flow. So, it's dependency should be the existence of auth code or something.

cerebrl · 2024-11-01T03:11:58Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+  const renderError = (errorNode) => {
+    const errorMessage = <ErrorMessage message={errorNode.error.message} key="error-message" />;
+    setComponents([errorMessage]);
+  };
+
+  // Represents the main render function for app
+  async function renderForm(nextNode) {


I'd just remove these two functions and place their contents in conditionals, rather than functions.

cerebrl · 2024-11-01T03:48:16Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+      if (collector.type === 'TextCollector' && collector.name === 'protectsdk') {
+        arrFormFields.push(
+          <Protect
+            collector={collector}
+            updater={client.current.update(collector)}
+            key={`protect-${collector.output.key}`}
+          />,
+        );
+      } else if (collector.type === 'TextCollector') {
+        arrFormFields.push(
+          <TextInput
+            collector={collector}
+            updater={client.current.update(collector)}
+            key={`text-${collector.output.key}`}
+          />,
+        );
+      } else if (collector.type === 'PasswordCollector') {
+        arrFormFields.push(
+          <Password
+            collector={collector}
+            updater={client.current.update(collector)}
+            key={`password-${collector.output.key}`}
+          />,
+        );
+      } else if (collector.type === 'SubmitCollector') {
+        arrFormFields.push(
+          <SubmitButton collector={collector} key={`submit-btn-${collector.output.key}`} />,
+        );
+      } else if (collector.type === 'FlowCollector') {
+        arrFormFields.push(
+          <FlowButton
+            collector={collector}
+            key={`flow-btn-${collector.output.key}`}
+            flow={client.current.flow({ action: collector.output.key })}
+            renderForm={renderForm}
+          />,
+        );
+      }


I do feel that this conditional logic can live in a function that's called on each item of an Array.map callback. In the existing React app, we have it as mapCallbacksToComponents.

cerebrl · 2024-11-01T03:58:22Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+  // Used for redirection after success
+  const navigate = useNavigate();
+
+  let client = useRef(null);


Let's see if we can remove the need for this useRef. To do that, let's move the davinci initialization out of the this component as it's a bit too "volatile", due to it's multiple rerenders. If we move it to a more stable component, and either add it as a React Context/Provider, or pass it down as an argument through the components.

cerebrl · 2024-11-01T03:59:57Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+    const code = successNode.client?.authorization?.code || '';
+    const state = successNode.client?.authorization?.state || '';


Even though this is technically valid, let's encourage the use of the getClient "selectors" to grab more specific data. This results in better types and better null exception handling.

Hi Justin, can you give me an example here. I'm not familiar getClient selectors. thanks.

Hey Jon, no worries! getClient is just a method on the DaVinci client instance. It returns the client portion of the current "node" data of the flow. You can see it being used here: https://github.com/cerebrl/davinci-client-demo/blob/main/main.ts#L35.

That's not to say what you did is wrong, but using the getClient "selector" just provides some conveniences over the directly object lookups.

thanks! guess I was on an older version of the client that didnt have that method. all set now.

Oh, yeah, that is definitely true. These "selectors" have been recently added.

cerebrl · 2024-11-01T04:03:51Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+    collectors.forEach((collector) => {
+      if (collector.type === 'TextCollector' && collector.name === 'protectsdk') {
+        arrFormFields.push(


I feel like the combination of Array.forEach and the Array.push within it is bit too "mutation driven". If we did something like this, I'd encourage the use of Array.map that returns a new array, rather than mutating an existing array with forEach. I hope that makes sense. When I'm using React, I think the more Functional Programming approach with its principle of immutability is best.

cerebrl · 2024-11-01T04:08:15Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+    if (newNode.status === 'next') {
+      renderForm(newNode);
+    } else if (newNode.status === 'success') {
+      return renderComplete(newNode);
+    } else if (newNode.status === 'error') {
+      return renderError(newNode);
+    } else {
+      console.error('Unknown node status', newNode);
+    }


To avoid having to redeclare this conditional logic multiple times, let's move this down into the return statement. To ensure this component rerenders, we use the fact that the async-await function above completes and new state is introduced to the component, either through a setState or from a custom React Hook, and that's what triggers a rerender and this conditional logic is rerun in the return statement.

refactor updates

updates from feedback

* move dv client in context * move dv client in context

Signed-off-by: jon-oblander <[email protected]>

ryanbas21 · 2024-11-11T20:36:17Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+      const node = await davinciClient.start();
+
+      if (node.status !== 'success') {
+        renderForm(node);


rename this?

ryanbas21 · 2024-11-11T20:37:01Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+  }
+
+  // Update the UI with the new node
+  async function renderForm(nextNode) {


This feels like a component, but its not.

Maybe this should be a custom hook?

ryanbas21 · 2024-11-11T20:37:08Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+    }
+  }
+
+  function mapRenderer(nextNode) {


Same as above

ryanbas21 · 2024-11-11T20:37:51Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+      {errorMessage.length > 0 && <ErrorMessage message={errorMessage} />}
+      {errorMessage.length == 0 &&
+        collectors.map((collector) => {
+          if (collector.type === 'TextCollector' && collector.name === 'protectsdk') {


I think this map callback should be abstracted, so its more clear to the reader what is happening in an isolated area.

ryanbas21 · 2024-11-11T20:38:37Z

reactjs-todo-dv/client/components/davinci-client/davinci-flow.js

+            return (
+              <Password
+                collector={collector}
+                updater={davinciClient.update(collector)}


Are we updating the collector in the render? Should this be done within the component, perhaps also inside of a hook?

ryanbas21 · 2024-11-11T20:41:39Z

reactjs-todo-dv/client/components/davinci-client/text-input.js

+import React, { useContext } from 'react';
+import { AppContext } from '../../global-state';
+
+const Text = ({ collector, updater }) => {


I see inconsistencies with how we are defining components. We should stick to one style. either arrows or function across the board.

ryanbas21 · 2024-11-11T20:42:38Z

reactjs-todo-dv/client/global-state.js

+  isAuthenticated,
+  prefersDarkTheme,
+  username,
+  loginClient,


do we want the loginClient to be passed in here? Seems like its a bit out of place and can be injected at build time below.

ryanbas21 · 2024-11-11T20:44:39Z

reactjs-todo-dv/client/index.js

@@ -9,18 +9,11 @@
 */

 import { Config, TokenStorage } from '@forgerock/javascript-sdk';
+import davinciClient from '@forgerock/davinci-client';


Not a note for this PR - but i'm wondering if we want to default export this. I typically prefer avoiding default exports to reduce problems on the consumer side. We can discuss this further but default exports i think arent good for tree shaking and code discoverability.

ryanbas21 · 2024-11-11T20:46:17Z

reactjs-todo-dv/client/views/login.js

-    methods.setAuthentication(true);
+  // This function is passed to DaVinciFlow to be called when the flow is complete
+  // Each individual flow might have a different completion action
+  function onComplete() {


Is this a Component definition, inside of the Login component definition?

ryanbas21 · 2024-11-11T21:36:34Z

reactjs-todo-dv/client/index.js

-    baseUrl: AM_URL,
-    timeout: '5000',
+    baseUrl: BASE_URL,
+    wellknown: `${BASE_URL}as/.well-known/openid-configuration`,


Should we be passing in just the base url or would it be better for the .env file to accept the whole openid configuration url?

jon-oblander added 5 commits October 29, 2024 10:00

duplicate reactjs-todo app: no code changes

8658abf

delete journey folder and files

95032b9

add davinci client files

6d955d8

additional app updates

6dcc782

missed debug code

9f0fe6d

cerebrl reviewed Oct 29, 2024

View reviewed changes

refactor

549feac

cerebrl requested changes Nov 1, 2024

View reviewed changes

jon-oblander and others added 6 commits November 1, 2024 07:47

Merge pull request #1 from jon-oblander/refactor

7ac725b

refactor updates

refactor updates

0a1a333

refactor updates

1c22455

Merge pull request #2 from jon-oblander/refactor

701155e

updates from feedback

move dv client to context (#3)

d74b129

* move dv client in context * move dv client in context

reset submit state

8070050

Signed-off-by: jon-oblander <[email protected]>

ryanbas21 requested changes Nov 13, 2024

View reviewed changes

		const [components, setComponents] = useState([]);
		const [pageHeader, setPageHeader] = useState('');


		let client = useRef(null);

		const renderComplete = async (successNode) => {

		const code = successNode.client?.authorization?.code \|\| '';
		const state = successNode.client?.authorization?.state \|\| '';

Reactjs todo dv new #14

Are you sure you want to change the base?

Reactjs todo dv new #14

Conversation

jon-oblander commented Oct 29, 2024

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment