-
Notifications
You must be signed in to change notification settings - Fork 707
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
Change comma by pipe to concat filters in the catalog #2279
Conversation
@@ -107,7 +107,7 @@ function Catalog(props: ICatalogProps) { | |||
useEffect(() => { | |||
const newFilters = {}; | |||
Object.keys(propsFilter).forEach(filter => { | |||
newFilters[filter] = propsFilter[filter]?.toString().split(","); | |||
newFilters[filter] = propsFilter[filter]?.toString().split("|"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can leave the ,
as a separator since it's a common practice, we don't need to change that. The thing is that we don't support (or we don't want to support?) arrays for the text search. I would just ignore the ,
for the text search here:
newFilters[filter] = filterNames.SEARCH === filter ? propsFilter[filter]?.toString() : propsFilter[filter]?.toString().split(",");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we need a test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is a good solution. Indeed, I did notice this problem when filtering operators; look at this comment: #2221 (comment)
What do you think? If implementing your solution, we will still have the same issue in operators (and there are plenty of companies with "," in their names...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, then it's better to properly encode the values rather than just making the bug less probable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oh no, encoding again!! 🙄 )
According to the RFC 3986?, the characters that may be subject to URL %-encoding are:
! # $ % & ' ( ) * + , / : ; = ? @ [ ]
But, they are considered sub-delimiters and there is no actual need to encode them even if encodeURIComponent does.
However, the qs
parser doesn't support it yet (ljharb/qs#237).
Indeed, qs
is decoding all the parameters, so we don't have control over the values we received.
For instance, even if I push the filters properly like Provider=something1,some%2Cthing2
, the component will always receive something1,some,thing2
, so the split/join won't work.
So, I don't think encoding it's a straightforward solution, perhaps we should dig deeper into the qs
module and re-implement the parsing function or directly substitute qs
with another alternative.
I'd come to an agreement with the delimiter character to use and would leave this PR as is (perhaps adding the ignore you mentioned for the text search as well) since it is "workarounding" an existing bug that prevents us from releasing.
Anyway, I'm open to implementing other alternatives, of course :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oh no, encoding again!! )
😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, so I would still like to maintain the ,
as a separator because it's the standard but it's true that encoding won't help. Let's replace ,
in the filters with a highly improbable string (I suggest a double underscore __
) and then undo the change when reading the filter. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I totally agree with this approach. Just a nitpick comment: if we use a non-to-be-encoded character like _
, the URL will be like: ...?Provider=Lightbend__%20Inc
(the underscore is visible).
What if we use another symbol that should be encoded', for instance, the whitespace character in braille: ⠀
(U+2800) (%E2%A0%80)
Anyway, I'm gonna start working with "__" and will update the PR asap.
FTR: : DNS-1123 subdomain spec doesn't allow either ,
or __
, it won't be a problem for the repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind what characters to use, use what you think is best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I would avoid the regexp but it's a minor thing. LGTM
@@ -107,7 +114,8 @@ function Catalog(props: ICatalogProps) { | |||
useEffect(() => { | |||
const newFilters = {}; | |||
Object.keys(propsFilter).forEach(filter => { | |||
newFilters[filter] = propsFilter[filter]?.toString().split("|"); | |||
const filterValue = propsFilter[filter]?.toString() || ""; | |||
newFilters[filter] = filterValue.split(",").map(a => a.replace(/__/g, ",")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's usually good to avoid regexps (they are more difficult to compute). You can do replaceAll("__", ",")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's was my first option indeed but... tests stop working! The problem is replaceAll
is not so widely supported, (see https://caniuse.com/mdn-javascript_builtins_string_replaceall). In nodejs, it's supported from >=15, so Jest/Enzyme complains about it's not a function.
So I just replace the replaceAll by a normal replace :P
Also, I think we should create the regex outside the loop indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:O thought that was there from before, nevermind then
Description of the change
Following up #2221 (comment) and #2264 (review).
This PR is to workaround this bug by simply change the default character for splitting and joining, the comma
,
, to another less frequent but also meaningful, the pipe|
.Benefits
Filters values having a comma as part of their names won't break the UI anymore:
Possible drawbacks
I don't think so, but I'm not fully aware whether or not this "comma"-split way to generate the URL is being used anywhere else.
Applicable issues
N/A
Additional information
N/A