[ui] Upgrade yaml library, improve config merge behavior #20055
Conversation
|
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 05ddc84. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 05ddc84. |
| const mergedYaml = yaml.stringify(updatedRunConfigData); | ||
|
|
||
| onSaveSession({runConfigYaml: mergedYaml}); | ||
| onSaveSession({runConfigYaml: mergeYaml(rootDefaultYaml, currentSession.runConfigYaml)}); |
There was a problem hiding this comment.
Use the set state action instead: onSaveSession((currentSession) => ({runConfigYaml: mergeYaml(rootDefaultYaml, currentSession.runConfigYaml)}));
This helps avoid bugs where multiple things try to write to the session in the same tick and end up overwriting each other meaning the last write wins.
There was a problem hiding this comment.
Hmm it looks like this should work (onSaveSession is a useSetStateUpdateCallback function reference, and that mimics SetStateAction), but we don't do it anywhere in the LaunchpadSession currently. Going to do do this in a small follow-up PR and change all the callsites at once
There was a problem hiding this comment.
edit: it looks like we're in a weird spot with this because onSaveSession takes a partial state (our previous solution to ensure it didn't get stale values), which means that the type on the current state is also the partial state, not the full state 🤔
There was a problem hiding this comment.
Can't we just set the full state and any fields we want to get rid of we set as undefined/null ? I do agree this should just take the full state though, we can update callsites to pass in the full state and they can preserve the old behavior by doing onSaveSession({...currentSession, {rootConfigYaml: ...}})
There was a problem hiding this comment.
Yeah I think we can -- we just need to read/write the full state using that spread operator everywhere. It actually looks like the function-setter won't work until we do that because the "current value" is just the last partial that was set currently. Was trying to see if I could change that, but just setting the entire state each time would be better.
Going to do this in a follow-up tonight, it's more lines of code than I expected!
js_modules/dagster-ui/packages/ui-core/src/launchpad/LaunchpadSession.tsx
Show resolved
Hide resolved
## Summary & Motivation This PR aims to address #19668 (but doesn't entirely fix it): Context: In the launchpad, there are several scenarios where we parse the current YAML, modify it as a JS object tree, and serialize it back to YAML. This is fundamentally undesirable because some YAML values (eg: 5.0) experience type coercion (eg: to 5) because JS does not have a Float type, etc. Fully fixing this would require changing the YAML merge logic to use YAML ASTs, but that's complex and we express config defaults as JS objects, so it wouldn't entirely resolve the "5.0 to 5" issue. This PR fixes two of the more straightforward bugs in the "parse, merge, stringify" workflow: - The deepmerge library changed in v3+ to concatenate arrays by default rather than combining them (so [a,b,c] + [c,d,e] = [a,b,c,c,d,e]). This changes it to the expected ([a,b,c,d,e]), and means that merging two identical arrays does nothing to them. - The yaml stringify method tries to be "lean" with quotes, but can un-quote some values that other YAML parsers parse as other types. The example called out in #19668 was "5:30" becoming 5:30, which is a Sexagesimal base-60 number in python. (This has been called out as awful behavior before: https://news.ycombinator.com/item?id=26679229). I changed the serialize call to explicitly maintain quotes on all string values, which looks a bit ugly if you're a YAML purist but will avoid these cases. ## How I Tested These Changes I broke out a yamlMerge helper and wrote tests for the cases above, verified they failed and then fixed them --------- Co-authored-by: bengotow <[email protected]>
## Summary & Motivation This PR aims to address #19668 (but doesn't entirely fix it): Context: In the launchpad, there are several scenarios where we parse the current YAML, modify it as a JS object tree, and serialize it back to YAML. This is fundamentally undesirable because some YAML values (eg: 5.0) experience type coercion (eg: to 5) because JS does not have a Float type, etc. Fully fixing this would require changing the YAML merge logic to use YAML ASTs, but that's complex and we express config defaults as JS objects, so it wouldn't entirely resolve the "5.0 to 5" issue. This PR fixes two of the more straightforward bugs in the "parse, merge, stringify" workflow: - The deepmerge library changed in v3+ to concatenate arrays by default rather than combining them (so [a,b,c] + [c,d,e] = [a,b,c,c,d,e]). This changes it to the expected ([a,b,c,d,e]), and means that merging two identical arrays does nothing to them. - The yaml stringify method tries to be "lean" with quotes, but can un-quote some values that other YAML parsers parse as other types. The example called out in #19668 was "5:30" becoming 5:30, which is a Sexagesimal base-60 number in python. (This has been called out as awful behavior before: https://news.ycombinator.com/item?id=26679229). I changed the serialize call to explicitly maintain quotes on all string values, which looks a bit ugly if you're a YAML purist but will avoid these cases. ## How I Tested These Changes I broke out a yamlMerge helper and wrote tests for the cases above, verified they failed and then fixed them --------- Co-authored-by: bengotow <[email protected]>
Summary & Motivation
This PR aims to address #19668 (but doesn't entirely fix it):
Context: In the launchpad, there are several scenarios where we parse the current YAML, modify it as a JS object tree, and serialize it back to YAML. This is fundamentally undesirable because some YAML values (eg: 5.0) experience type coercion (eg: to 5) because JS does not have a Float type, etc. Fully fixing this would require changing the YAML merge logic to use YAML ASTs, but that's complex and we express config defaults as JS objects, so it wouldn't entirely resolve the "5.0 to 5" issue.
This PR fixes two of the more straightforward bugs in the "parse, merge, stringify" workflow:
The deepmerge library changed in v3+ to concatenate arrays by default rather than combining them (so [a,b,c] + [c,d,e] = [a,b,c,c,d,e]). This changes it to the expected ([a,b,c,d,e]), and means that merging two identical arrays does nothing to them.
The yaml stringify method tries to be "lean" with quotes, but can un-quote some values that other YAML parsers parse as other types. The example called out in Incorrect parsing of string-type sexagesimal values from launchpad #19668 was "5:30" becoming 5:30, which is a Sexagesimal base-60 number in python. (This has been called out as awful behavior before: https://news.ycombinator.com/item?id=26679229). I changed the serialize call to explicitly maintain quotes on all string values, which looks a bit ugly if you're a YAML purist but will avoid these cases.
How I Tested These Changes
I broke out a yamlMerge helper and wrote tests for the cases above, verified they failed and then fixed them