refactor global config merging strategy on upgrade #1031
Merged
Conversation
This file contains 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
oshi97
force-pushed
the
dev/global-config-ugprade-refactor
branch
from
1342b8b
to
326d7bb
Compare
This commit refactors how global configs are merged on theme upgrades. Now, global configs will always have the same "structure" as the incoming config, instead of sometimes placing properties in seemingly random spots, if the two configs don't match up exactly in terms of structure. Per Rose's request, the sdkVersion is now always updated to the incoming config's value. Besides that, preexisting properties will favor the original config, while commented out properties will favor the incoming config. Properties and comments in the original that do no exist in the incoming config will be moved to the bottom of the config. J=SLAP-1717 TEST=manual,auto ran an upgrade
oshi97
force-pushed
the
dev/global-config-ugprade-refactor
branch
from
326d7bb
to
04340fc
Compare
tmeyer2115
reviewed
Jan 6, 2022
postupgrade/mergeGlobalConfigs.js
Outdated
| const beforeComments = commentJsonObject[beforeSymbol] || [] | ||
| const afterComments = commentJsonObject[afterSymbol] || [] | ||
| beforeComments.forEach(handleComment) | ||
| const inlineComment = afterComments.find(c => c.inline); |
There was a problem hiding this comment.
The interior of this forEach loop is a bit difficult to follow. What are the beforeComments and afterComments exactly? Are these the commented out properties that occur in a specific block of the config? That specific block is keyed by key?
How are normal, un-commented properties getting added to the list of tokens? Could we update some names here to make clearer what's happening?
There was a problem hiding this comment.
should be a bit clearer now though it's inherently a little low level
tmeyer2115
approved these changes
Jan 6, 2022
cea2aj
approved these changes
Jan 6, 2022
tmeyer2115
approved these changes
Jan 6, 2022
Merged
tmeyer2115
added a commit
that referenced
this pull request
Feb 8, 2022
### Features - Multi-lang versions of the `document-standard` result card and the `documentsearch-standard` direct answer card have been added. (#1041, #1039) - We automatically bump the `sdkVersion` in the `global_config` on upgrade. (#1031) ### Changes - RTF styling was added to the `event-standard`, `faq-accordion`, and `location-standard` cards. (#1037) - The default `universalSectionTemplate` for a `vertical-grid` page was updated to `grid-three-columns`. (#1038) ### Bugfixes - When clicking outside the iFrame'd experience, the query suggestions dropdown now closes. (#1034) - There was a bug with the custom `card` command. If the new card's name contained the name of the forked card, the former would not be added properly. This is now fixed. (#1036) - Previously, if there was a Map on the Universal page, a user could get stuck on the Map while scrolling down the page. This has been fixed. (#1030, #1042)
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 commit refactors how global configs are merged on theme upgrades.
Now, global configs will always have the same "structure" as the incoming
config, instead of sometimes placing properties in seemingly random spots,
if the two configs don't match up exactly in terms of structure.
Per Rose's request, the sdkVersion is now always updated to the incoming
config's value. Besides that, preexisting properties will favor the
original config, while commented out properties will favor the incoming config.
Propertys andCommentedOutPropertys in the original that do not exist in the incoming configwill be moved to the bottom of the config.
Comments that are neither inline nor
CommentedOutPropertys will be ignored gracefully, and an error message displayed.J=SLAP-1717
TEST=manual,auto
ran an upgrade from 1.26 to this branch
ran an upgrade from 1.26 to this branch, but with the global config changed to have all values replaced with new ones, and with an extra prop at the bottom