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: support object/array-formed condition (#212) #213

Conversation

RexSkz
Copy link
Contributor

@RexSkz RexSkz commented Oct 14, 2023

This PR handles the following scenario:

  • If two tests are string, regex, function, use the previous logic (compare the toString result).
  • If they are objects or arrays, flatten them and sort the entries, then do the compare.

The order of arrays in tests should not affect the result, e.g. [/a/, /b/] and [/b/, /a/] are the same test. To handle this, we can replace numbers in flattened keys (the numbers are definitely from arrays). For example, a.1.b will be replaced with a.[].b.

Closes #212


To let someone with interests understand it quickly, here is an example:

// Original
[
  { and: [/a/, /b/] },
  /c/,
]
[
  /c/,
  { and: [/b/, /a/] },
]
// After flatten
{
  '0.and.0': /a/,
  '0.and.1': /b/,
  '1': /c/,
}
{
  '0': /c/,
  '1.and.0': /b/,
  '1.and.1': /a/,
}
// After using `Object.entries` and replacing numbers with `[]`
[
  ['[].and.[]', /a/],
  ['[].and.[]', /b/],
  ['[]', /c/],
]
[
  ['[]', /c/],
  ['[].and.[]', /b/],
  ['[].and.[]', /a/],
]
// After the sort
[
  ['[]', /c/],
  ['[].and.[]', /a/],
  ['[].and.[]', /b/],
]
[
  ['[]', /c/],
  ['[].and.[]', /a/],
  ['[].and.[]', /b/],
]

Finally, we just compare each item: use the === for [0], and use the toString for [1].

@RexSkz RexSkz force-pushed the rex/issue-212/merge-with-rules-test-equivalent branch from b58f98d to 6ad282d Compare October 14, 2023 12:11
@bebraw bebraw merged commit b4dc4d3 into survivejs:develop Oct 16, 2023
5 checks passed
@RexSkz RexSkz deleted the rex/issue-212/merge-with-rules-test-equivalent branch October 16, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mergeWithRules misjudges the rules.test equivalent
2 participants