Angular todo dv #18
Angular todo dv #18
Conversation
| updateValue(value: string, collector: Collector): void { | ||
| const updater = this.davinciClient.update(collector); | ||
| updater(value); | ||
| } |
There was a problem hiding this comment.
This is a bit of an anti-pattern from what we're intending with this update function. What we recommend is calling this davinciClient.update() method when you are iterating over the collectors, passing the result of the method execution to the collector component. This keeps the collector data and the method to update it isolated to the collector component.
This would also provide the benefit of not having to have as much indirection related to emitting events and listening for events to call other functions. I hope that makes sense.
There was a problem hiding this comment.
I could see the argument being made for either situation. As an alternative viewpoint, what if a customer has a collection of input/button components already and they don't want to add an entirely new component that's dependent on our SDK, it's reasonable to assume they'd have them set up with a label and an output of the value when it's changed. It kind of goes against angular's design patterns and principle to tightly couple your components to things like that. My "code smell" senses start tingling when complex objects and callbacks are being passed around Angular.
I intentionally tried to keep the davinci-flow component focused on managing the state of the flow and progressing nodes and stuff.
If you'd prefer me to make this change I will but I did want to explain why I did it this way first.
There was a problem hiding this comment.
Thanks for explaining the intent behind your implementation. I have to admit I am not an Angular expert, so take what I say with a grain of salt.
I tried to find more information on passing functions to child components, but I honestly couldn't find good information on it. It seems there are many ways to pass functions and methods to components. I do see that the major complaint is the binding of the this keyword when sharing a method of the parent class with the child component.
I guess I can imagine the issue with this is the iteration of collectors is done within the template, and not within the JavaScript context like React. So, is there no way to call davinciClient.update(collector) during iteration time, passing the returned value (a function) to the component. Is that the issue?
Just to be clear, the function that is returned from the davinciClient.update(collector), is solely for the collector component. The parent component has no need for it at all. So, this isn't parent-child communication per se, but literally just creating an isolated function during iteration for each collector's own use. The collector component calls that update function and the internal store of the DaVinci Client updates the value of that collector.
Anyway, if you still feel strongly about the pattern used, I'm okay with it as you likely have more experience with Angular than I do. I appreciate your patience with me :)
There was a problem hiding this comment.
Yeah with Angular your templates and javascript are completely separate so you can't always use the same patterns as with React. They (angular team) have strong opinions about how to do stuff which I don't always agree with, there's a reason I made a career swift away from Angular :).
I made a slight refactor which moves creating those update functions into the iteration loop, I'm okay with that. I am going to stand my ground on not making the form fields dependent on the SDK though.
There was a problem hiding this comment.
Sure, this seems like a good compromise. No need to stand your ground, though. We trust your opinion :)
There was a problem hiding this comment.
These collector components don't seem to have any error handling on them at all. When we get a node type of error, we should grab the cached response from the endpoint and pull the input level error message from it to indicate the error for correction. Right now, I think we're just treating all non-successes as failures, which require restarting the flow entierly.
There was a problem hiding this comment.
I noticed this as well, I ASSumed it was a current limitation of the SDK or something. So we do display a general form error but then we hide all the collector fields when an error is encountered.
So I may be doing something wrong but in order to get the error message I need to call this.davinciClient.next() and at that point the collectors start throwing not found errors and I can't update the values and attempt to resubmit the node.
There was a problem hiding this comment.
No, you are not doing anything wrong per se; it's just not very obvious how to handle this. So, what you have to do is call next and receive the new node, but you need to hold on to the previous node, which contains the collectors. If the new node is an error (not failure), you need to reuse the old node and render both the error message along with the old node containing its collectors.
After correcting the error, you should be able to submit the previous node with the new values. I hope that makes sense.
There was a problem hiding this comment.
Thanks for the clarification, that makes sense, just have other priorities at the moment, I'll get that updated later today or tomorrow and let you know!
There was a problem hiding this comment.
Thanks! Take your time; we're just happy that you're able to help :)
There was a problem hiding this comment.
I attempted to revisit this today. First of all I tried to update the davinci-client to v1.0.0 and it is throwing a typescript error.
I was able to workaround this by manually changing that reference in node_modules/@forgerock/davinci-client/dist/lib/config.types.d.ts from @forgerock/javascript-sdk/src/config/interfaces to @forgerock/javascript-sdk/dist/config/interfaces, but looks like you may need to updated something with your typings output or something. Once I did that everything built just fine.
But I am unable to submit with the next function after I encounter an error. Note in the screenshot I am displaying the error and the form fields, and in the console you can see that the update function is being called but there are errors when I try to use the collector to update the values. Then the SDK seems to be doing a postback instead of trying to submit the data to the endpoint.
There was a problem hiding this comment.
I attempted to revisit this today. First of all I tried to update the davinci-client to v1.0.0 and it is throwing a typescript error.
I was able to workaround this by manually changing that reference in
node_modules/@forgerock/davinci-client/dist/lib/config.types.d.tsfrom@forgerock/javascript-sdk/src/config/interfacesto@forgerock/javascript-sdk/dist/config/interfaces, but looks like you may need to updated something with your typings output or something. Once I did that everything built just fine.But I am unable to submit with the
nextfunction after I encounter an error. Note in the screenshot I am displaying the error and the form fields, and in the console you can see that the update function is being called but there are errors when I try to use the collector to update the values. Then the SDK seems to be doing a postback instead of trying to submit the data to the endpoint.
I checkout out the last commit from this branch and I was able to build without any changes. However, the type error your getting is most likely because of the ts configuration.
I would gander that the tsconfig.json (or tsconfig.app.json) needs to be moduleResolution: Bundler
This is partially the SDKS fault, but in this scenario, this would be the correct module setting for the angular app because it is the host application, which is using a bundler which would be responsible for the module resolution / linking.
There was a problem hiding this comment.
In regards to the latter problem, do you have a har file that can show this or can you put here a simple few reproduction steps so I can try to replicate this?
There was a problem hiding this comment.
Updating the moduleResolution fixed that problem and it looks like new angular projects are created with that bundler flag so I'm good with that fix, but it may be worth noting we used the non davinci angular sample as a starting point so you guys may want to make that update there too.
As for the second problem I've attached a zip with the har and a .env file. You can use this .env file and run ng serve to reproduce, you can create an account through the flow its pointed to test. You just need to submit invalid username and password, then update to the valid username/password and try to submit again. If you run it make sure pull the latest again.
There was a problem hiding this comment.
Thanks. Looking through this it seems like a bug. Although there are ways around this that kind of make it not breaking, this isn't working the way would intended it to be.
What is happening (tell me if i'm wrong) - you are purposefully submitting a bad username/password.
So in this instance you get back an error node, but we are fully replacing the continue node state with the error node state.
What we should do is probably intelligently merge the states so you don't lose access to the previous continue node.
What we have to find out is which set of interaction and id tokens we should keep around since they seem to change.
We will look into this thanks
|
Closing in favor of #47 |
Angular Sample for davinci client
https://pingidentity.atlassian.net/browse/SDKS-3195