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

Interactions: Run conditionally based on query param #18706

Merged
merged 5 commits into from
Aug 2, 2022

Conversation

JonathanKolnik
Copy link
Contributor

@JonathanKolnik JonathanKolnik commented Jul 14, 2022

The original intention of checking if the instrumenter code is mounted within in an iframe was to accomplish 2 things:

  1. Don't run the instrumentation if it's not in a context (the manager) where the interactions panel could be shown
  2. Make sure there is a window.parent that is different than the child so that the state can be preserved when the child refreshes.

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

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jul 14, 2022

☁️ Nx Cloud Report

CI 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 target

Sent with 💌 from NxCloud.

@JonathanKolnik JonathanKolnik force-pushed the run-interactions-conditionally branch from 650db84 to f0af160 Compare July 14, 2022 15:05
@shilman shilman changed the title allow interactions to run conditionally when interactions query param… Interactions: Run conditionally based on query param Jul 14, 2022
@JonathanKolnik JonathanKolnik marked this pull request as ready for review July 15, 2022 14:28
@JonathanKolnik JonathanKolnik requested a review from shilman July 15, 2022 14:28
@JonathanKolnik
Copy link
Contributor Author

@shilman I see that these test failures are on multiple branches at this time, I believe they are unrelated to this change.

@JonathanKolnik
Copy link
Contributor Author

JonathanKolnik commented Jul 15, 2022

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 __STORYBOOK_ADDON_INTERACTIONS_INSTRUMENTER_STATE__['addons-interactions-accountform--standard-email-failed'] has data but it's incomplete calls object (length is only 3)

[
    {
        "id": "addons-interactions-accountform--standard-email-failed [0] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 0,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onSubmit"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [1] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 1,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onTransactionStart"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [2] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 2,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onTransactionEnd"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    }
]

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):

[
    {
        "id": "addons-interactions-accountform--standard-email-failed [0] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 0,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onSubmit"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [1] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 1,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onTransactionStart"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [2] action",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 2,
        "path": [],
        "method": "action",
        "args": [
            {
                "__function__": {
                    "name": "onTransactionEnd"
                }
            }
        ],
        "interceptable": false,
        "retain": true,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [3] within",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 3,
        "path": [],
        "method": "within",
        "args": [
            {
                "__element__": {
                    "prefix": null,
                    "localName": "div",
                    "id": "root",
                    "classNames": [],
                    "innerText": "Create an account to join the Storybook community\n\nEmail\nPassword\nCREATE ACCOUNT\nRESET"
                }
            }
        ],
        "interceptable": false,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [4] getByTestId",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 4,
        "path": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [3] within"
            }
        ],
        "method": "getByTestId",
        "args": [
            "email"
        ],
        "interceptable": false,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [5] type",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 5,
        "path": [
            "userEvent"
        ],
        "method": "type",
        "args": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [4] getByTestId",
                "retain": false
            },
            "gert@chromatic"
        ],
        "interceptable": true,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [6] getByTestId",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 6,
        "path": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [3] within"
            }
        ],
        "method": "getByTestId",
        "args": [
            "password1"
        ],
        "interceptable": false,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [7] type",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 7,
        "path": [
            "userEvent"
        ],
        "method": "type",
        "args": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [6] getByTestId",
                "retain": false
            },
            "supersecret"
        ],
        "interceptable": true,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [8] getByRole",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 8,
        "path": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [3] within"
            }
        ],
        "method": "getByRole",
        "args": [
            "button",
            {
                "name": {}
            }
        ],
        "interceptable": false,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [9] click",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 9,
        "path": [
            "userEvent"
        ],
        "method": "click",
        "args": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [8] getByRole",
                "retain": false
            }
        ],
        "interceptable": true,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [10] findByText",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 10,
        "path": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [3] within"
            }
        ],
        "method": "findByText",
        "args": [
            "Please enter a correctly formatted email address"
        ],
        "interceptable": true,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [11] expect",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 11,
        "path": [],
        "method": "expect",
        "args": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [0] action",
                "retain": true
            }
        ],
        "interceptable": true,
        "retain": false,
        "status": "done"
    },
    {
        "id": "addons-interactions-accountform--standard-email-failed [12] toHaveBeenCalled",
        "storyId": "addons-interactions-accountform--standard-email-failed",
        "cursor": 12,
        "path": [
            {
                "__callId__": "addons-interactions-accountform--standard-email-failed [11] expect"
            },
            "not"
        ],
        "method": "toHaveBeenCalled",
        "args": [],
        "interceptable": true,
        "retain": false,
        "status": "done"
    }
]

@yannbf
Copy link
Member

yannbf commented Jul 18, 2022

Hey @ghengeveld whenever you're back, could you take a look at this PR? Thank you <3

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);
Copy link
Member

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.

// 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')) {
Copy link
Member

@ghengeveld ghengeveld Jul 26, 2022

Choose a reason for hiding this comment

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

Suggested change
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.

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.

👍

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.

Copy link
Member

@shilman shilman left a 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.

@JonathanKolnik
Copy link
Contributor Author

@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.

@shilman
Copy link
Member

shilman commented Jul 28, 2022

@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

@JonathanKolnik
Copy link
Contributor Author

@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?

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Aug 2, 2022
@shilman shilman merged commit c6d9ea9 into next Aug 2, 2022
@shilman shilman deleted the run-interactions-conditionally branch August 2, 2022 22:41
shilman added a commit that referenced this pull request Aug 2, 2022
…ally

Interactions: Run conditionally based on query param
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: interactions feature request patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants