-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add support to provide namePrefix and tags for filtering #237
Conversation
test/url-utils.test.js
Outdated
|
||
test('geturl should return url with two tags if two tags are provided', t => { | ||
const result = getUrl('http://unleash-app.com', '', '', ['tagName:tagValue', 'tagName2:tagValue2']); | ||
t.true(result === 'http://unleash-app.com/client/features?tag=tagName:tagValue&tag=tagName2:tagValue2'); |
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 this will work out of the box with express. In other examples we have been using empty square bracket for array bases query params like this:
http://unleash-app.com/client/features?tag[]=tagName:tagValue&tag[]=tagName2:tagValue2
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.
What do you recommend to do with it?
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 tried reading up on the standards and it seems to be quite unclear both brackets
and repeat
format seems to be supported by express (unleash-server).
I do however think it will be worth while to urlEncode the tagName:tagValue
params
src/url-utils.ts
Outdated
} | ||
if (tags) { | ||
tags.forEach((tag) => { | ||
params.append('tag', tag); |
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 should do a encodeURIComponent
like this:
params.append('tag', encodeURIComponent(tag));
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.
Nice to get this in to the SDK 👍🏼
Only fix the encoding of the tag values and I think we are good to go!
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 removed the extra decodeURIComponent from uri-utils.ts
that should not have been there. All good now!
No description provided.