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(smart-field): smart-field search use returned query #1058

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JustinMartinDev
Copy link
Contributor

@JustinMartinDev JustinMartinDev commented Jan 10, 2023

Definition of Done

General

Actually smart-field search use mutation on query params instead of use returned query value as defined in type definition and in doc.

⚠️ My proposal is to use the returned value, but keep mind it will break the search of people using only mutation.

If you want to keep the mutation, update doc to remove the return query and in type definition change return to void

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)
  • Ensure that Types have been updated according to your changes (if needed)

Security

  • Consider the security impact of the changes made

@Scra3
Copy link
Member

Scra3 commented Jan 11, 2023

Hello really thanks for your contribution.
Indeed there is inconsistency. What was the motivation for your contribution? Did you encounter a bug?
🙏

@JustinMartinDev
Copy link
Contributor Author

Yes, to avoid any side-effect due to mutation, I created a new object "query" with my additional condition and return it, but my condition was ignored.

After exploring the search-builder, i saw the smart-field search use mutation instead of type definition & doc.

const advancedQuery = {
    ...query,
    where: {
      ...where,
      [Op.and]: [
        {
          ...first,
          [Op.or]: [...first[Op.or], myCondition],
        },
        ...rest,
      ],
    },
  };

@Scra3
Copy link
Member

Scra3 commented Jan 11, 2023

We have released a v9 3 months ago and a new node-js agent
We will update the documentation so for now we're putting your PR aside, it may come in handy. 🙏

We will update the documentation and the TS interface.

Ty for your contribution.

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.

2 participants