Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix updatePreset behavior #10212

Merged
merged 6 commits into from Dec 2, 2021
Merged

Fix updatePreset behavior #10212

merged 6 commits into from Dec 2, 2021

Conversation

Oreilles
Copy link
Contributor

@Oreilles Oreilles commented Dec 2, 2021

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use merge here, as the nested value of layout_query & _options is a JSON blob of unknown type. Lodash's merge does a deep-merge by default, causing conflicts when merging nested options objects from layouts.

Take the following example:

import _ from 'lodash';

const before = {
  layout_query: {  
    tabular: {
      fields: ['id', 'title']
    },
    cards: {
      fields: ['id']
    }
  }
};

const update = {
  layout_query: {
    tabular: {
      fields: ['author']
    }
  }
};

const afterMerge = _.merge({}, before, update);

// {
//   layout_query: {
//     tabular: { fields: [ 'author', 'title' ] },
//     cards: { fields: [ 'id' ] }
//   }
// }

In this case, the fields for tabular should've been just ['author'].


The bug that currently exists in assign is that the "scope" is overwritten, for example the result by using assign instead of merge is:

{
  layout_query: { tabular: { fields: [ 'author' ] } }
}

which is correct for tabular, but is missing the previous settings for cards.


To properly fix this, we'll have to use assign, but on the first nested level (as per my comment). For example:

import _ from 'lodash';

const before = {
  layout_query: {  
    tabular: {
      fields: ['id', 'title']
    },
    cards: {
      fields: ['id']
    }
  }
};

const update = {
  layout_query: {
    tabular: {
      fields: ['author']
    }
  }
};

const afterNestedAssign = _.assign(
  {},
  before,
  {
    layout_query: _.assign(
      {},
      before.layout_query,
      update.layout_query
    )
  }
);

// {
//   layout_query: {
//     tabular: { fields: [ 'author' ] },
//     cards: { fields: [ 'id' ] }
//   }
// }

@Oreilles
Copy link
Contributor Author

Oreilles commented Dec 2, 2021

Thanks for taking the time to explain this, I still had not got it right.. Should be fixed now 👍

@rijkvanzanten rijkvanzanten merged commit 81ccf49 into main Dec 2, 2021
@rijkvanzanten rijkvanzanten deleted the fix/use-preset branch December 2, 2021 20:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants