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

Handle boolean values #1

Closed
wants to merge 3 commits into from
Closed

Handle boolean values #1

wants to merge 3 commits into from

Conversation

br0wn
Copy link

@br0wn br0wn commented Mar 26, 2018

At the moment boolean values are transformed to string (ex: false becomes "false").

This PR handles boolean values as special case and does not set any value if boolean value is false which is how HTML form handles checkboxes.

Do not set any value if boolean value is false
@codecov
Copy link

codecov bot commented Mar 26, 2018

Codecov Report

Merging #1 into master will decrease coverage by 2.95%.
The diff coverage is 50%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1      +/-   ##
=======================================
- Coverage   86.95%   84%   -2.96%     
=======================================
  Files           2     2              
  Lines          23    25       +2     
=======================================
+ Hits           20    21       +1     
- Misses          3     4       +1
Impacted Files Coverage Δ
serialize.js 95% <50%> (-5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa4d54...a0ddd49. Read the comment docs.

@octet-stream
Copy link
Owner

octet-stream commented Mar 26, 2018

Hi! Thanks for your PR first.

The different serializers makes boolean transformations differently. So, I don't think it is really necessary thing, because you can always write a converter on the server that will handle boolean values. Do you?

@octet-stream octet-stream requested review from octet-stream and removed request for octet-stream March 29, 2018 12:45
@br0wn
Copy link
Author

br0wn commented Apr 2, 2018

From https://www.w3.org/TR/html401/interact/forms.html#checkbox

Checkboxes (and radio buttons) are on/off switches that may be toggled by the user. A switch is "on" when the control element's checked attribute is set. When a form is submitted, only "on" checkbox controls can become successful.

A successful control is "valid" for submission.

From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

Note: If a checkbox is unchecked when its form is submitted, there is no value submitted to the server to represent its unchecked state (e.g. value=unchecked); the value is not submitted to the server at all.

I would argue that following standard HTML form submitting procedure would provide most suitable developer experience. Since boolean value most appropriately translates to a checkbox control, transforming it to FormData value should follow same procedure as native HTML forms do.

@octet-stream
Copy link
Owner

octet-stream commented Apr 2, 2018

I understood your point. Following standards wasn't a general purpose for me, when I have made this library. It's just a tiny object/collection serializer which is more like JSON.stringify, but for transforming them to form-data and work together with then-busboy, which is transforms booleans to their original type by defalut.

Anyway, I can make this case configurable by adding strict mode, if you need it. Are you argree with that?

Also, please be consider to send pull requests from different branches, not from master (something like br0wn/patch-1). I thought it was common GitHub recommendation. Maybe I need to add contributing guidelines even for this little library.

octet-stream added a commit that referenced this pull request Apr 2, 2018
@octet-stream
Copy link
Owner

I've implemented your proposal as serialization option. See API section in documentation for more information.

@br0wn
Copy link
Author

br0wn commented Apr 16, 2018

@octet-stream Thank you very much :)
I apologize for the incorrect PR, I neglected to check the guidelines for proper PR flow.
Will do it properly next time. Cheers!

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