Skip to content
This repository has been archived by the owner on May 17, 2024. It is now read-only.

[MAJOR] add matching keys functional constraint #41

Merged
merged 2 commits into from
May 4, 2018
Merged

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Apr 10, 2018

Adds a new functional constraint to validate the fact that the top-level key in a trigger/search/create matches the key value. Failing to do this causes a wild bug where the trigger is in the definition twice.

This is because in core, we run the following in compileApp (link), which copies properties onto the wrong key if they don't match. A simple example:

const o = { a: { key: "a" }, b: { key: "c" } };

_.each(o, val => {
  o[val.key] = { checked: true, ...val };
});

JSON.stringify(o, null, 2)

yields:

{
  "a": {
    "checked": true,
    "key": "a"
  },
  "b": {
    "key": "c"
  },
  "c": {
    "checked": true,
    "key": "c"
  }
}

where c is in the definition twice, but one is a bad version.

See: initial slack report

@xavdid xavdid requested a review from eliangcs April 10, 2018 07:51
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

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

Pulled and confirmed it works as expected! I only have one question on the code. Thanks!

// if none of the actionTypes are in this, we don't have a top-level definition
if (!_.some(actionTypes.map(t => definition[t]), Boolean)) {
return [];
}
Copy link
Member

@eliangcs eliangcs Apr 11, 2018

Choose a reason for hiding this comment

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

Doesn't looks like this if block is necessary. The for loop that follows next should safely exit even definition doesn't have any of actionTypes. Or am I missing something?

@xavdid xavdid merged commit bba5922 into master May 4, 2018
@xavdid xavdid deleted the matching-keys branch May 4, 2018 05:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants