Skip to content

refactor: re-organise regular (non A11y) queries by predicate #965

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

Merged
merged 15 commits into from
May 4, 2022

Conversation

mdjastrzebski
Copy link
Member

@mdjastrzebski mdjastrzebski commented Apr 29, 2022

Summary

Re-organise internal queries code by predicate typ #325

Scope of changes

  • Replace GetByAPI, QueryByAPI and FindByAPI types and functions with per query type and functions (ByXxxQueries, bindByXxxQueries)
  • Move query implementations from src/helpers to src/queries
  • Simplified query type definitions while making query options generic to allow greater flexibility

Note: Changes consist of moving types around and tweaking exports. In order to reduce PR size and potential for bugs I refrained from doing any query logic changes.

Note 2: This is the reason why I intend to make a separate PR for A11y queries as there we would require some code changes.

Note 3: There are certain inconsistencies when it comes to existence (or not) of certain deprecated query aliases. I did not make any changes but kept the existing status as is. Let me know if I should remove old aliases or fill in the missing ones

Note 4: We have really awesome automated tests!

Test plan

  • All tests pass (no tests changed)
  • TypeScript and Flow checks pass

@mdjastrzebski mdjastrzebski changed the title Refactor/query by predicate finishing touches refactor: re-organise regular (non A11y) queries by predicate Apr 29, 2022

// Removed aliases
// TODO: remove in the next release
queryByName: () => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be in the unsafeByName file I guess (same for the other mentions of byName in the rest of the file)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unmodified name from before the PR and it's there for easier code migration for users.

That being said this code deprecation is quite old now, all users should have migrated a long time ago, so I would be for removing these legacy aliases. @thymikee @AugustinLF wdyt about remove all these deprecated aliases?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm all for removing deprecated functions

Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed all removed functions error messages.

Copy link
Collaborator

@AugustinLF AugustinLF left a comment

Choose a reason for hiding this comment

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

Looks good to me, glad that we're getting this in :)

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

🚀

@thymikee thymikee merged commit 64ff2d2 into main May 4, 2022
@thymikee thymikee deleted the refactor/query-by-predicate-finishing-touches branch May 4, 2022 10:08
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.

4 participants