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

feat: Accept single filter function rather than dict #9

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

mike-plummer
Copy link

@mike-plummer mike-plummer commented Apr 29, 2024

CP-2790

Updates made in #8 added the ability to provide filter functions to conditionally exclude certain trait values from being used during selector generation. In a nod to performance these were structured to only call a filter function if the trait being considered exactly matched the one under consideration (exact attribute name match, usually). However, it is desired that filters be able to supply a regex to match traits (e.g. allow filtering multiple attributes with the same criteria) which necessitates a change in contract to supply a single filter function that encapsulates all the necessary filtering logic. This will have a small performance impact on selector generation since every single trait will need to call into this filter function, but impact can be mitigated by aggressive use of the supplied type and key params as well as appropriate caching/memoization when crafting the filter param.

Note: I had originally planned to handle caching internally here (since we already are doing so for overall selector gen and isUnique checks) but doing so required pulling in a lot of knowledge of how filters are being structured for our specific use case. I backed that out to reduce coupling and believe caching can be handled as efficiently/elegantly by the caller, but if anyone sees a superior approach I'd be interested to hear it since this lib has significant performance implications for UICov.

Once this merges there will be follow-on changes in cypress-services side to update this dep and build an appropriate filter function off of config

Before

unique(
  element,
  {
    filters: {
      'ng-type': (type, key, value) => value !== 'test',
      'ng-kind': (type, key, value) => value !== 'test'
    }
  }
)

After

unique(
  element,
  {
    filter: _.memoize(
      (type, key, value) => {
        if (type === 'attribute' && /^((ng-type)|(ng-kind))$/.test(key)) {
          return value !== 'test'
        }
        return true
      },
      (type, key, value) => `${type}${key}${value}`
    )
  }
)

src/index.js Outdated Show resolved Hide resolved
Copy link

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Makes sense to me - wanted to confirm we're releasing a new major version (2.0.0) for this?

Co-authored-by: Tyler Biethman <[email protected]>
@mike-plummer
Copy link
Author

Makes sense to me - wanted to confirm we're releasing a new major version (2.0.0) for this?

@tbiethman Yes, this is a breaking change so I will mark this as such on merge which should trigger a major version bump

@mike-plummer mike-plummer merged commit 5b5afe4 into master Apr 30, 2024
3 checks passed
@mike-plummer mike-plummer deleted the mikep/CP-2790-regex-attr-filters branch April 30, 2024 13:32
Copy link

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants