-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Interactions: Run conditionally based on query param #18706
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f0af160. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
650db84
to
f0af160
Compare
@shilman I see that these test failures are on multiple branches at this time, I believe they are unrelated to this change. |
This may be incomplete tldr; do we need to emit an event that will run the complete interaction? Details: If you visit https://5a375b97f4b14f0020b0cda3-giqthgsxkj.chromatic.com/iframe.html?args=&id=addons-interactions-accountform--standard-email-failed&viewMode=story&interactions=true You can see that the
vs. compared to what we actually want which is what is in the normal view https://5a375b97f4b14f0020b0cda3-giqthgsxkj.chromatic.com/?path=/story/addons-interactions-accountform--standard-email-failed complete calls object (length is 13):
|
Hey @ghengeveld whenever you're back, could you take a look at this PR? Thank you <3 |
lib/instrumenter/src/instrumenter.ts
Outdated
if (global.window.parent === global.window) return obj; | ||
// Don't do any instrumentation if not loaded in an iframe and it's not running in a capture. | ||
if (global.window.parent === global.window) { | ||
const params = new URLSearchParams(global.window.location.search); |
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.
Will this work in IE? Not sure a polyfill is generated.
lib/instrumenter/src/instrumenter.ts
Outdated
// Don't do any instrumentation if not loaded in an iframe and it's not running in a capture. | ||
if (global.window.parent === global.window) { | ||
const params = new URLSearchParams(global.window.location.search); | ||
if (!params.get('interactions')) { |
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.
if (!params.get('interactions')) { | |
if (!params.get('instrument')) { |
It's not just for interactions, and interactions
suggests it would enable/disable the actual interactions which it doesn't.
I'm thinking perhaps we should support the inverse as well. So if you set instrument=false
then instrumentation will be disabled even if loaded within an iframe. In other words, the instrument
param is respected (true
or false
), and if not specified we'll fall back to the window.parent check.
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.
👍
Not sure where the search params (full
, singleStory
, etc) are documented, but this one should be added there.
Also perhaps we should propagate the query param from index.html to iframe.html so this can be used even when running with the manager UI. Update: This actually seems to already work.
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.
@JonathanKolnik I wonder if this is the right place to put this code. There is also an Options
object that is passed to the instrumenter and instrument
could be a key on that, and set based on the query param at some place higher up in the code. I don't feel too strongly about this either way, but global vars are a code smell.
@shilman that is a good point that I hadn't considered yet, but as I start to look into it I am hesitant to make this change because then I would need to update @storybook/testing-lib, @storybook/jest, and any other lib that is using instrument with this new option. If you feel like that is still the right approach though, I'll move ahead with the refactor. |
@JonathanKolnik I'm ok with the current change. Path of least resistance! Just wanted to throw the suggestion out there, though, in case it resulted in a better solution |
@shilman totally! Thanks for helping me through this one. Let me know what else I can do from here. Also will we be able to patch this into 6.5? |
…ally Interactions: Run conditionally based on query param
The original intention of checking if the instrumenter code is mounted within in an iframe was to accomplish 2 things:
This PR adds an override to this logic block, because there are some instances where the metadata itself is still valuable even if the panel is not shown. As for the window.parent, since this is not getting reloaded, it is of less consequence that they are equal.
Issue:
You couldn't access the
__STORYBOOK_ADDON_INTERACTIONS_INSTRUMENTER_STATE__
from the iframe.html view of a story.What I did
Added an additional check for the presence of query param,
interactions
, that would circumvent the early ejection of the instrumenter code.How to test
If your answer is yes to any of these, please make sure to include it in your PR.