-
-
Notifications
You must be signed in to change notification settings - Fork 732
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
Parse booleans & numbers #91
Comments
My current feeling is that this belongs elsewhere. There are so many conversion patterns that could take place that I don't really want to open the can of worms of implementing them all here. Unless there's a large demand for this, I'm going to say no. Leaving open for discussion, however. |
OK, I made a plugin for now https://github.com/xpepermint/query-types. |
I think the @xpepermint request is valid. I was disappointed to see that this isn't a feature. That parse assumes all numbers are strings makes it not so helpful. |
This is actually a problem I am facing using the very popular https://github.com/request/request library. What I am running into is that my REST api is outputting booleans as |
Part of me wants this too, but automatic type coersion here is troublesome. What if you wanted In particular if you're allowing user input to show up in the query string, all someone has to do is type in At least the current behavior is predictable and very well defined. |
Thanks for the information. I am no longer blocked by this, so I think I can get by without this change. I was actually able to get around this using https://github.com/visionmedia/superagent instead of https://github.com/request/request (which uses querystring for posts). I can only assume that |
The plugin created by @xpepermint worked for me and will likely work for others who need it. I agree with the fact that I don't think |
I'm happy to consider a way for users to opt in to automatic coercion - but I wouldn't want that by default. I'm closing this for now, but am happy to reopen it if it warrants further discussion or work. |
Could there not be some way of indicating coercion on a case-by-case basis, using a special character maybe? For example, we could have http://example.com/api/todos/?completed@=true&id@=1, and that would indicate that we want We could have the special character either in the property name, or in the value itself, and we could put it either at the beginning or the end (although I like it right up next to the = sign). [ update ] I've got the above implementation working here, at least for parsing... we might want to add some enforcement for the character(s) that can be chose to indicate coercion, which I currently have defaulting to @; |
That seems very inelegant - you'd need to support that special character in your server code as well as your client code. In other words, there's no standard for using some kind of special character, so I think it'd be a very bad idea to codify that into |
Can't say I agree... is the []= syntax standard? It's commonly used, but I don't think it's any kind of standard, as far as I can see, yet qs supports it... plus it lets you pick a non-standard delimiter... which you'd need to support in your server code as well as your client code. Besides, choosing a character isn't a huge ask, especially when the default is provided. People can just construct their urls in the client using @=, and then the server would coerce just those values. If they want to always coerce, they can easily set the coercion character to an empty string. Anyways, it's your tool. I might go ahead and spin something off. |
It is specified in http://www.w3.org/TR/html-json-forms/ , yes. It was working being done to create a standard, through that work has been stopped now. |
I was here with the same issue and I just solved it like:
|
Adding my +1 to this feature request and linking it to the previous discussion in the original package that There's definitely a lot of support and use cases for the feature, and I think making it opt-in would be great with a |
@mmcgahan You can already provide your own encoder and decoder, deciding for yourself when "true" means a boolean versus a string - I'm not sure how that decision could ever be codified inside |
ah, |
Hi, I'm actually disappointed that this issue is closed. I see so many issues raising the same boolean problem. Heres my suggestion: its ok to not have this behavior by default, we can just have { allowBooleans: true } options to qs.parse() just like many other existing options out there.. |
@izelnakri again, you can already do this yourself: #91 (comment) The issue remains that "true" might be the string "true" or a boolean true; there's no way to know for sure, so since that knowledge is highly specific to your application and your codebase, the only solution that makes any sense is for that knowledge to live in your application and your codebase - via a custom encoder/decoder. |
@ljharb Seems like this issue still gets the occasional +1 because there is demand in the community for a basic level of decoding built into this package on an opt-in basis. The "true" string case comes up so often, and in most cases is so obviously something that you'd want coerced, that people want a common package that will do such basic decoding. I'd bet that 99% of the time, coercing "true" to a boolean would be preferable. I'd make a similar bet about coercing "123" to an integer. Given that these are such common use cases, I think it makes at least some sense that there might be an opt-in option in the common querystring builder that we all use. If everyone (or 99% of everyone) is implementing these basic coercion cases in their own application-relevant decoders, it would save a lot of lines of code in the long run. I'm a little more educated now re: standards, and I understand the reluctance to set a de facto standard by implementing it in this library. Maybe a separate package that provides a basic extensible decoder is a better solution (I haven't looked for one recently). However, I still think that including it as an opt-in option (i.e. a non-standard option) in this package as @izelnakri suggests, avoids this problem, because it's not the default functionality: It's just a very lightweight enhancement that allows basic encoding/decoding if you want to use it. That's my two cents, but obviously I'm way past this issue by now. |
It's pretty trivial to please most use cases by using the qs.parse(data, {
decoder(value) {
if (/^(\d+|\d*\.\d+)$/.test(value)) {
return parseFloat(value);
}
let keywords = {
true: true,
false: false,
null: null,
undefined: undefined,
};
if (value in keywords) {
return keywords[value];
}
return value;
}
}); |
Also with our decoder code, by using |
this doesn't seem to work for me. |
found the solution that helped me, instead of passing this: |
Generally agree with @ljharb here, maybe a 1 line note in the main readme.md to summarise how types are cast would help. Also accept the point that pasting the decoder function around is a pain and not that pretty. I would export a locally augmented version of qs in your project in this case rather than a new wrapper npm package (if that's what was meant by @waynebloss). |
SOLUTIONExtending the default
|
With @crobinson42 solution what would you suggest to do if you really want to have a param encoded as a string even if it could be decoded as a number? For example you have an input field where a user can search for something. If the user wants to search for just a number (e.g. an ID) it will be parsed as a number but your input and filter logic is expecting a string. It will be really painful to handle this edge cases everywhere. |
with all this conversation above I still have no idea how to achieve this. |
@AndreiMotinga based off of #91 (comment): qs.parse(window.location.search, { ignoreQueryPrefix: true, arrayFormat: 'bracket', decoder(str, decoder, charset) {
const strWithoutPlus = str.replace(/\+/g, ' ');
if (charset === 'iso-8859-1') {
// unescape never throws, no try...catch needed:
return strWithoutPlus.replace(/%[0-9a-f]{2}/gi, unescape);
}
if (/^(\d+|\d*\.\d+)$/.test(str)) {
return parseFloat(str)
}
const keywords = {
true: true,
false: false,
null: null,
undefined,
};
if (str in keywords) {
return keywords[str]
}
// utf-8
try {
return decodeURIComponent(strWithoutPlus);
} catch (e) {
return strWithoutPlus;
}
} }); |
Here is something a bit more configurable with additional options such as function newDecoder (parseNumbers = true, parseBool = true, ignoreNull = true, ignoreEmptyString = true) {
return function decoder (str, decoder, charset) {
const strWithoutPlus = str.replace(/\+/g, ' ')
if (charset === 'iso-8859-1') {
// unescape never throws, no try...catch needed:
return strWithoutPlus.replace(/%[0-9a-f]{2}/gi, unescape)
}
if (parseNumbers && /^(\d+|\d*\.\d+)$/.test(str)) {
return parseFloat(str)
}
if (ignoreEmptyString && str.length === 0) {
return
}
const keywords = {
null: ignoreNull ? undefined : null,
undefined
}
if (str in keywords) {
return keywords[str]
}
const boolKeywords = {
true: true,
false: false
}
if (parseBool && str in boolKeywords) {
return boolKeywords[str]
}
try {
return decodeURIComponent(strWithoutPlus)
} catch (e) {
return strWithoutPlus
}
}
} |
When i use your solution with
When i edit this line and add |
@sfaujour a custom decoder is required to only return a string. |
isn't it good to have an option as parseBool and parseNumber just like query-string have? |
That package's docs for parseBool say "if it's a boolean", and "if it's a number", but don't explicitly describe how they determine that. In some server frameworks, a boolean is yes/no/true/TRUE/false/FALSE/on/off/1/0. In others, it may just be true/false. In some, it might be cierto/falso, perhaps. In other words, having these options that arbitrarily encode one person's opinion about "how to determine them" is flawed at best, and I'd prefer not to have them at all. You can, however, use a custom decoder to decide for yourself what "it's a boolean" means, and then qs wouldn't run the likely risk of having nonsensical options. |
@ljharb |
The thing is, this type of "auto-coercion" code must be written to iterate over data and interpret certain fields as scalar/primitive values (numbers, booleans, etc). That code can be written in two places:
Option 2 is a clear winner hear. I propose that the docs should link to a code tried-and-true coercion snippet for easy copy/paste and lay this issue to rest. |
Happy to accept PRs to improve the docs. |
Note that this check for floats ( |
Oh! I thank it's very helpful~ 👍 |
This comment was marked as outdated.
This comment was marked as outdated.
It's work for me qs.parse(request.querystring, {
decoder(str: string, defaultDecoder: qs.defaultDecoder, charset: string, type: 'key' | 'value') {
if (type === 'value' && /^(?:-(?:[1-9](?:\d{0,2}(?:,\d{3})+|\d*))|(?:0|(?:[1-9](?:\d{0,2}(?:,\d{3})+|\d*))))(?:.\d+|)$/.test(str)) {
return parseFloat(str);
}
const keywords = {
true: true,
false: false,
null: null,
undefined: undefined,
};
if (type === 'value' && str in keywords) {
return keywords[str];
}
return defaultDecoder(str, defaultDecoder, charset);
},
}) |
Currently I use
express-query-boolean
package which parsestrue
andfalse
strings into boolean but it would be great to have this feature included inqs
.The text was updated successfully, but these errors were encountered: