Node: Safe use of `document` for preview #24248

DylanPiercey · 2023-09-20T22:43:44Z

What I did

Avoid referencing document.location when it's possible for it not to be there.

I am currently using the following apis in order to reimplement the testing portion for Marko Storybook which I am upgrading to support storybook 7.

import {
  composeStories,
  composeStory,
  setProjectAnnotations,
} from "@storybook/preview-api";

These defer the implementation of the story composition for testing which is great!

The issue I ran into is that the @storybook/preview-api uses a browser api which does not exist in server environments and for Marko apps (likely Svelte, Solid and anything else with a different ssr compilation) it is nice to also test these fixtures in a server context.

With the simple change in this PR it seems this API works fine in a bare node environment and there is already precedent in the storybook codebase for checking if the document.location exists.

Checklist for Contributors

Testing

I'm not exactly sure how best to add tests for this but am open to ideas.

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

Use any storybook project where the stories could be rendered on the server.
Import the stories, and the composeStory api in a node script.
Try and render the story using the composeStory api.

-->

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
- bug: Internal changes that fixes incorrect behavior.
- maintenance: User-facing maintenance tasks.
- dependencies: Upgrading (sometimes downgrading) dependencies.
- build: Internal-facing build tooling & test updates. Will not show up in release changelog.
- cleanup: Minor cleanup style change. Will not show up in release changelog.
- documentation: Documentation only changes. Will not show up in release changelog.
- feature request: Introducing a new feature.
- BREAKING CHANGE: Changes that break compatibility in some way with current major version.
- other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

…esting api

DylanPiercey · 2023-09-20T22:52:35Z

I found the main issue is calling @storybook/preview-api#start. So another work around for me is to just avoid that in the testing setups.

I'm happy with that work around, but I'll leave up the PR if you all wish to discuss.

ndelangen · 2023-09-21T12:59:49Z

code/lib/preview-api/src/modules/preview-web/UrlStore.ts

@@ -22,7 +22,7 @@ const getQueryString = ({
  selection?: Selection;
  extraParams?: qs.ParsedQs;
 }) => {
-  const { search = '' } = document.location;
+  const search = (document && document.location && document.location.search) || '';


I'm ok with this defensive programming, just to be on the safe side.

Minor nit pick: could you use optional chaining here to keep the code concise?

@shilman WDYT?

Happy to use optional chaining, I only avoided it to stick with the pattern used here: https://github.com/storybookjs/storybook/blob/next/code/lib/preview-api/src/modules/client-api/queryparams.ts#L7

We should start using optional chaining there too 😅

@ndelangen do you think it's worth changing this? Or create a separate issue for switching stuff to optional chaining more broadly?

ndelangen · 2023-09-21T13:00:35Z

I am currently using the following apis in order to reimplement the testing portion for Marko Storybook which I am upgrading to support storybook 7.

OW! that's awesome! Please reach out to me, if you need assistance!

tmeasday · 2023-09-22T08:18:03Z

@DylanPiercey I'm confused as to why that code is executing if you are only using the listed APIs?

yannbf · 2023-09-22T12:02:20Z

@DylanPiercey I'm confused as to why that code is executing if you are only using the listed APIs?

This happens when APIs like composeStories are called in node, in non JSDOM environments. We had a similar issue when trying composeStories with Playwright CT. This PR seems like it would fix those problems!

tmeasday · 2023-09-23T04:33:18Z

This happens when APIs like composeStories are called in node, in non JSDOM environments. We had a similar issue when trying composeStories with Playwright CT.

What does composeStories call that ends up calling these lines?

This PR seems like it would fix those problems!

Agree, but it seems like maybe a bandaid. I don't think a preview should be getting initiated if there is no "preview", at least naively (maybe there's a good reason it needs to). Can we figure out if it absolutely necessary for these lines to execute?

DylanPiercey · 2023-09-23T04:37:03Z

@tmeasday to be clear calling composeStory or composeStories does not cause these lines to execute. What does is calling the start api from preview. Since in my case start was called unconditionally and the test apis were exposed through the same module this is more of a side effect.

I changed it to lazily call start so that it doesn't happen in testing, which is a good enough work around for me, but maybe this change is still worth while?

tmeasday · 2023-09-23T04:52:09Z

What does is calling the start api from preview. Since in my case start was called unconditionally and the test apis were exposed through the same module this is more of a side effect.

There's no reason to call start outside of story store v6 which is definitely not relevant in this scenario. We'll be removing that code in SB8.0 in any case.

I changed it to lazily call start so that it doesn't happen in testing, which is a good enough work around for me, but maybe this change is still worth while?

Can you explain this to me / link me to this code?

tmeasday · 2023-11-28T03:25:00Z

@ndelangen although this PR isn't causing any harm, I'm still unsure why it's necessary. Plus I wonder if we need to check typeof document !== 'undefined' rather than just !!document, TBH.

ndelangen · 2023-11-28T06:54:55Z

I agree @tmeasday

ndelangen · 2024-03-26T09:10:25Z

@yannbf considering you've worked a lot on composable stories.. Do you think this PR should still be merged?

yannbf · 2024-03-26T09:14:44Z

@yannbf considering you've worked a lot on composable stories.. Do you think this PR should still be merged?

I don't think these changes are much needed anymore because we don't instantiate PreviewWeb like we used to in Storybook 7, which was part of exporting clientApi. However I think it's fine to merge them, so I'd say let's do it! But would be nice to change the assertions to use optional chaining if possible

nx-cloud · 2024-03-26T09:38:18Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 779117c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 1 target

nx run-many -t build -c production --parallel=8

Sent with 💌 from NxCloud.

fix: avoid relying on document for preview file used in the compose/t…

556b02e

…esting api

DylanPiercey requested review from yannbf, ndelangen and tmeasday as code owners September 20, 2023 22:43

ndelangen reviewed Sep 21, 2023

View reviewed changes

ndelangen changed the title ~~fix: avoid relying on document for preview file used in the compose/t…~~ Node: Safe use of document for preview Sep 22, 2023

ndelangen self-assigned this Sep 22, 2023

ndelangen added bug preview-api ci:normal labels Sep 22, 2023

Merge branch 'next' into avoid-document-reference

45ab3c2

Merge branch 'next' into avoid-document-reference

6243f1c

ndelangen approved these changes Nov 24, 2023

View reviewed changes

ndelangen removed their assignment Nov 27, 2023

ShaunEvening assigned tmeasday and ndelangen Nov 27, 2023

ndelangen removed their assignment Mar 26, 2024

ndelangen added 2 commits March 26, 2024 10:28

Fix document undefined error in UrlStore and WebView

830890c

Merge branch 'next' into pr/DylanPiercey/24248

779117c

ndelangen approved these changes Mar 26, 2024

View reviewed changes

ndelangen merged commit 11534e4 into storybookjs:next Mar 26, 2024
51 checks passed

github-actions bot mentioned this pull request Mar 26, 2024

Release: Prerelease 8.1.0-alpha.5 #26611

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node: Safe use of `document` for preview #24248

Node: Safe use of `document` for preview #24248

DylanPiercey commented Sep 20, 2023

DylanPiercey commented Sep 20, 2023

ndelangen Sep 21, 2023

DylanPiercey Sep 21, 2023

ndelangen Sep 21, 2023

DylanPiercey Sep 22, 2023

ndelangen commented Sep 21, 2023

tmeasday commented Sep 22, 2023

yannbf commented Sep 22, 2023 •

edited

Loading

tmeasday commented Sep 23, 2023

DylanPiercey commented Sep 23, 2023

tmeasday commented Sep 23, 2023

tmeasday commented Nov 28, 2023

ndelangen commented Nov 28, 2023

ndelangen commented Mar 26, 2024

yannbf commented Mar 26, 2024

nx-cloud bot commented Mar 26, 2024 •

edited

Loading

Node: Safe use of document for preview #24248

Node: Safe use of document for preview #24248

Conversation

DylanPiercey commented Sep 20, 2023

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

DylanPiercey commented Sep 20, 2023

ndelangen Sep 21, 2023

Choose a reason for hiding this comment

DylanPiercey Sep 21, 2023

Choose a reason for hiding this comment

ndelangen Sep 21, 2023

Choose a reason for hiding this comment

DylanPiercey Sep 22, 2023

Choose a reason for hiding this comment

ndelangen commented Sep 21, 2023

tmeasday commented Sep 22, 2023

yannbf commented Sep 22, 2023 • edited Loading

tmeasday commented Sep 23, 2023

DylanPiercey commented Sep 23, 2023

tmeasday commented Sep 23, 2023

tmeasday commented Nov 28, 2023

ndelangen commented Nov 28, 2023

ndelangen commented Mar 26, 2024

yannbf commented Mar 26, 2024

nx-cloud bot commented Mar 26, 2024 • edited Loading

☁️ Nx Cloud Report

Node: Safe use of `document` for preview #24248

Node: Safe use of `document` for preview #24248

yannbf commented Sep 22, 2023 •

edited

Loading

nx-cloud bot commented Mar 26, 2024 •

edited

Loading