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

Fix boolean arg types parsing and encoding #21102

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Feb 15, 2023

Closes #20960

What I did

Convert string boolean type to boolean type.

How to test

  1. Run a random sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Open Storybook in your browser
  3. Access ADDONS controls/basics/Undefined story
  4. Set the boolean value
  5. Refresh the page. The boolean value should be actually true or false, instead of "true" or "false"

Rationale

When the current serialization/parsing logic for URLs was implemented, we mostly relied on argTypes to be able to parse things back to the correct type. However, those types aren't always available or might be a union type (e.g. string | number) so that's insufficient. We introduced !null and !undefined and those color things in order to be able to represent those in the URL (how else could you?).
We assumed booleans would be handled via argTypes, so we did not introduce a special serialization syntax for those.

Of course, we considered the various standard serialization syntaxes, including JSON. However, JSON is actually a terrible fit for URLs because it uses quotes all over the place (which are super ugly in URLs) and simply removes undefined values as if they were never there (try JSON.stringify({ foo: undefined })). So we decided to stick with what qs provides and build a dialect on top to handle these special data types.

In this PR, we continue to use our own dialect on top of qs to support also boolean values. This solution of course isn't bug-free (the string !true will be converted into a boolean `true) We consider this as an absolute edge-case. In the future, we might revisit this decision to use JSON parsing instead (although we would sacrifice readability).

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Args bool type will be string type after refresh page.
2 participants