refactor(core): Reactify overrideTimeout #7126
Conversation
|
Converting the
|
| const [overrideTimeout, setOverrideTimeout] = useState(false); | ||
| const [configurable, setConfigurable] = useState(false); | ||
| const [defaults, setDefaults] = useState({ hours: 0, minutes: 0 }); | ||
| const helpContent = HelpContentsRegistry.getHelpField('pipeline.config.timeout'); |
There was a problem hiding this comment.
Thanks so much for doing this, Jammy!
I am having a little bit of a hard time understanding the data flow here -- is there a reason we need to store all these values on state and handle updating the stage config with useEffect? My (naive) understanding is that useEffect is mainly intended for more side-effect-y operations previously done in componentDidMount / componentDidUpdate, and that whenever possible it's a bit more straightforward to directly call methods to update the model in response to whatever action should change the data. In general I feel like components like this should be as dumb as possible, receive the minimum props necessary to display what they need to display, and not store anything on state that can be derived from the props they receive. But very open to new paradigms now that we have hooks and super curious for @christopherthielen's wisdom here!
There was a problem hiding this comment.
The reason I needed useEffect was when a user chooses a different stage type in a different component it would result in different stageConfig (b8bd46c#diff-f0f41e28f10844cbe8077af8dfc669edR255)
which would cause the trigger of setOverrideValues
You are right, I think I could rework it a little bit to have it store fewer things in state, I was just focused on lifting the previous logic implemented and porting it over.
There was a problem hiding this comment.
I was just focused on lifting the previous logic implemented and porting it over.
I think this is a good approach, when possible. There are definitely different paradigms when using angular and react (and the libraries within each ecosystem). I'd like to shift our codebase to use more react-friendly paradigms. However, if we do both things in one commit (port from angular to react and refactor), it can be very difficult to later track down regressions in behavior. I believe it's safer long term to migrate the existing logic first in one commit, and then later simplify in another.
There was a problem hiding this comment.
Got it, appreciate the explanation Jammy! 👍
There was a problem hiding this comment.
In this particular case, the usage of useEffect looks appropriate to me. We want to be able to respond to changes to the stageConfig and perform some "expensive" computations. This aligns with both Don't stop the data flow and Lazy initial state.
There was a problem hiding this comment.
I believe it's safer long term to migrate the existing logic first in one commit, and then later simplify in another.
Makes sense to me! And thanks for the links, will read 📖
| const [overrideTimeout, setOverrideTimeout] = useState(false); | ||
| const [configurable, setConfigurable] = useState(false); | ||
| const [defaults, setDefaults] = useState({ hours: 0, minutes: 0 }); | ||
| const helpContent = HelpContentsRegistry.getHelpField('pipeline.config.timeout'); |
There was a problem hiding this comment.
I was just focused on lifting the previous logic implemented and porting it over.
I think this is a good approach, when possible. There are definitely different paradigms when using angular and react (and the libraries within each ecosystem). I'd like to shift our codebase to use more react-friendly paradigms. However, if we do both things in one commit (port from angular to react and refactor), it can be very difficult to later track down regressions in behavior. I believe it's safer long term to migrate the existing logic first in one commit, and then later simplify in another.
| const [overrideTimeout, setOverrideTimeout] = useState(false); | ||
| const [configurable, setConfigurable] = useState(false); | ||
| const [defaults, setDefaults] = useState({ hours: 0, minutes: 0 }); | ||
| const helpContent = HelpContentsRegistry.getHelpField('pipeline.config.timeout'); |
There was a problem hiding this comment.
In this particular case, the usage of useEffect looks appropriate to me. We want to be able to respond to changes to the stageConfig and perform some "expensive" computations. This aligns with both Don't stop the data flow and Lazy initial state.
…94 (#7150) * chore(core): Bump version to 0.0.377 d5425b4 refactor(core): virtualize execution rendering (#7140) 342087c feat(core): allow users to override pipeline graph positions (#7141) fb58d70 fix(core): do not send a cloud provider on v2 search calls (#7142) 5d6d9aa chore(core): clarify clone stage help text (#7131) 745f0a1 fix(core): Display latest template in pipeline template list (#7145) 456172b fix(webhooks): addresses issue 3450 - introduce a delay before polling webhook (#7144) 7238d1d feat(core): Enable new artifacts workflow in bakeManifest (#7138) 45e5aa3 feat(artifacts): find multiple artifacts from single execution (#7139) dae73da fix(core): provide key for repeating param JSX elements (#7136) bbc1d06 fix(core): filter falsy error messages from errors object on tasks (#7135) 4782808 refactor(core): Reactify overrideTimeout (#7126) 0a3bd68 feat(core/presentation): Always call onBlur in Checklist to "mark as touched" (#7134) cdd6f23 chore(package): Just Update Prettier™ 7e464a9 fix(amazon): Support SpEL in advanced capacity (#7124) 08e9506 chore(deck): Update to Typescript 3.4 fa515dc fix(core): do not stretch provider logos in selection modal (#7128) * chore(docker): Bump version to 0.0.42 8358010 fix(docker): Allow auto-switch to manual entry when refreshing images (#7120) * chore(amazon): Bump version to 0.0.194 cdd6f23 chore(package): Just Update Prettier™ 7e464a9 fix(amazon): Support SpEL in advanced capacity (#7124) 08e9506 chore(deck): Update to Typescript 3.4
No description provided.