storybook: API.setQueryParams not works as expected
Describe the bug
Expected URL updated when api.setQueryParams
get called.
To Reproduce Steps to reproduce the behavior:
- In any add-on tooltip component, where get access with
storybookAPI
, callstorybookAPI.setQueryParams({ foo: 'bar' })
.
Expected behavior
URL change to, for example: http://localhost:6006/?path=/story/test&foo=bar
.
System:
- OS: MacOS Mojave
- Device: MacBook Air 2013-mid
- Browser: Chrome 72
- Framework: React 16.8.1
- Version: v5.0.0-beta.2
Additional context Related #5271
if call the following immediately
console.log(api.getQueryParam('foo')) // `bar` printed
Therefore, it is working (previously in alpha.11 is not working though), but the URL is not changing. Also, by directly change the URL state like http://localhost:6006/?path=/story/test&foo=bez
. 'bez'
get printed correctly, but the URL changed to http://localhost:6006/?path=//story/test
. It added additional slash after ?path
mysteriously… And if refresh the page, no 'bez'
get printed.
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Comments: 34 (21 by maintainers)
If the API is called set/get query params, you kinda expect it to work that way 😃 How about instead of changing its behavior, this API is slowly deprecated over several releases? That would probably be better for devs expecting the API to work like it did before (myself included).
@tmeasday Here gives me some confusions, does any reason we don’t want the URL keep the query parameter?! Ideally, I would like to share the URL to other people, let’s say for designer. That would be super useful for our communication.
The biggest reason for me to have query in URL is for testing. I have add-on work for this (e.g.
&themes=dark&languages=english
), this gives us the ability to mock a particular environment state that the component is reacted to (i.e theming or language). It would be nice we can have this kept when we “open the component into a new tab”, then we can copy past URL without manually typing those parameters.In our team, we want to have at least 1 story per dummy components and they are smartly knobbed and auto-generated. For some special variants (i.e. component specific) we will have more granular-defined stories to document some important use case so we can cherry-pick them for smoke testing.
@tmeasday
And? Who cares? That’s the entire point of query params. If I want to send a URL to a co-worker, with the exact state of the story I’m looking at, the only way to do that is with query params. At the moment, it’s impossible and it forces my co-worker to go through the same steps I did to get to the same outcome.
This is even harder to accomplish when using addons or custom implementations.
I’m somewhat disappointed by this change.
Mainly because there seems to be an over-estimated value of “pretty” URLs. And I think that is a subjective perspective – some people value sharability of state more than pretty URLs.
Makes me wonder if having an app-level setting makes sense – similar to the
Show sidebar
andShow addons
settings. Something likeSharable urls
.Additionally, I think there is something not being considered in this issue (and in the solution proposed in #6486) which is the difference between queries in the preview and manager apps.
In my use case (which I understand is likely different than others’) I’d only like to set the query url of the manager app. There is a state that influences how my addon renders (not the story) that I’d like to make sharable. The current feature where users can get the full URL by opening the preview app (iframe) separately doesn’t totally work for me because as I said, what I’d like to make sharable is a link to the manager app to the tab of my addon, with the modified state.
One thought that I’d like to share to support different treatment of preview queries and manager queries is the already existing difference between query params for the selected story (Referring to
iframe.html?id=story-id
vs?path=/story/story-id
)I think having control over the query params on both of the apps would be a great way to empower addon developers.
Something like
Hey devs, any ETA on when we can expect backwards-compat behavior?
@andyindahouse You should be able to listen on the channel every time the knobs update:
https://github.com/storybooks/storybook/blob/next/addons/knobs/src/KnobManager.js#L89
Then you should be able to serialize the knob values yourself:
https://github.com/storybooks/storybook/blob/next/addons/knobs/src/components/Panel.js#L107
This could all change in future releases though, so do this at your own risk. This isn’t a use-case of knobs, and is actually different from what @leoyli is asking in the original issue.
@leoyli We’re addressing this in the knobs addon as follows:
Would you be able to do something similar to support your use case?
Son of a gun!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.25 containing PR #6486 that references this issue. Upgrade today to try it out!
Because it’s a pre-release you can find it on the
@next
NPM tag.Closing this issue. Please re-open if you think there’s still more to do.
Ok, I’ve been doing some digging and coding… so there are 2 things I’ve been working on today: It’s about to get technical…
a few facts:
I think there’s 2 things to do:
1 - make customQueryParameters useful again.
And it seems people want to use them for accessing specific story states. This means the customQueryParameters must be accessible in the iframe.
2 - make management of addon state easy and convenient, so they too can make use of persistence, and other useful things.
First I’m going to address the customQueryParameters.
Second I have some new API in mind for setting addon state:
@ndelangen,
I’m also good with the above too. But I hope the “opening on a standalone tab with the current state” can also put on the table (query params are working there but just not approachable…). Reason as I’ve mentioned.
About the store part, my initial concerns is a bit on mess up the storybook internal state so it became hard to debug but maybe I just worries for no reason. After some thoughts, I have also think this could be a great idea since so far I’m relying on singleton pattern for managing
addon-contexts
state. Have an official way to persist the state may unlock some potential for addon creators.Sorry for the delay on my part.
What if we allowed addons to put their state into the main storybook state? This would help addon creators, since they can more easily combine their own state, with the state of other addons & storybook’s internal state using the
Consumer
-component.But also, because there’s now a centralized state for all these things, we can easily serialize that state, and temporarily drop it into the url for sharing and load this state when the url contains this data (and then clean the url).
This would help addon creators and users and maintainers at the same time.
There’d be no api for setting some url, that isn’t actually shown, only an api for setting state.
Feedback welcome @shilman @tmeasday
@milesj Thanks for your patience on this & flexibility on the approach. URL handling got messed up in various ways in the v5 release and didn’t manage to get fix in the post-release bug bash. I’ll make sure it gets handled soon – on vacation this week (supposedly) but will put more urgency here (aside from just labeling as
high priority
, which apparently didn’t magically solve the problem 😆)@shilman is that “copy URL” feature different from the actual browser URL?
I concur with @tmeasday , if a use case requires URL copy/paste, it seems an anti pattern to have both a real browser URL that does not contain every params, and another “secondary URL” component, either shown in the UI or hidden with a button that says “copy URL”? (even though it is not copying browser URL, but something else instead)
I also agree with @milesj on an opt-in solution to let the user decide if they would like to use query params in the URL or not.
I have a feeling that this change basically favour aesthetics over actual feature. Prioritization of Form Over Function, if you will.
I think the URLs should be clean and we should allow people to get the “full” URL for sharing.
Steps to make this happen:
@milesj Would this be an acceptable solution from your point of view?
I think there is a tension here with URLs becoming hideous and unreadable. Potentially there is still an argument for specific addons to write to the URL, maybe we should support that via
setQueryParam
or even.setState(.., { persistence: 'url' })
.@ndelangen I think was the driver of removing this from the URL. I am interested to hear his thoughts.