remove potential for uncaught mismatch #97
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KeeyanGhoreshi
added a commit
that referenced
this pull request
Feb 1, 2024
plarocque4
pushed a commit
that referenced
this pull request
Feb 8, 2024
* make settings a modal and add save button * remove potential for uncaught mismatch (#97) * fix checkboxes * fix issue with patient load
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This does not necessarily need to be pulled in, but could help address problems with our settings. One issue we have is that we define the settings in two places, once in data.js as a definition, and then again in the RequestBuilder's state. This produces the potential for a bug where the values between the two are mismatched, and since the SettingsBox only looks at the headerDefinitions, then tries to set the state, it could end up setting the state for a non-existent value. This error would not produce an exception and the program would just appear to not work for no reason, making this a dangerous possible error.
The solution is to define one "source of truth" which I've done by pushing the headerDefinitions all into data.js, which RequestBuilder now uses to define its state. The downside of this is that the RequestBuilder's state is now built dynamically, and can't be referenced directly (by intellisense, for example) meaning you might not know what exactly is in the state when programming. A bug here WOULD produce an error, however, so it's easily detectable.
In an idealized version of this fix, the state would hold its own definition of its settings, which would be passed to settingsBox as a prop, which would then render them. This would require a bit of work, however, to restructure how the app uses and updates its settings, as well as how SettingsBox renders it.
I don't think it's worth the effort to do all that, though. In fact, even this change might not be worth it, given that it makes things less convenient in return for solving an issue that's very unlikely to occur.