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

Price Range filter on Vtex legacy PLP #191

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jovenan
Copy link

@jovenan jovenan commented Aug 11, 2023

Feature description

Allows Price Range filter on Vtex legacy ProductListingPage:
image

By default Vtex sends a JSON like this to loader:
image
But we can't use this url format in deco, because it returns an error, as it can't handle filters of this type.

In this PR we use the fq=P:[{a} TO {b}] to filter by the range prices registered in the vtex.

Copy link
Contributor

@mcandeia mcandeia 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, but I would try to remove all uses of regex. Except for certain types of encryption, regex is the second slowest CPU-intensive operation one can perform. I only use regex in cases where it's absolutely necessary, for instance, if you can transform it into startswith/endswith/includes, that would be ideal. Keep in mind that this runs for each request and is CPU-bound. Since Deno is single-threaded, the main thread gets occupied processing regex. And even though it might seem insignificant, when you have hundreds and thousands of simultaneous requests, it ends up making a difference of milliseconds per request.

@jovenan jovenan requested a review from mcandeia August 12, 2023 00:00
@jovenan
Copy link
Author

jovenan commented Aug 12, 2023

@mcandeia Thanks for review. I made these changes.

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