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

Core: Support custom PREVIEW URL for block story iframe #16773

Merged
merged 18 commits into from
Dec 9, 2021

Conversation

ittaibaratz
Copy link
Contributor

@ittaibaratz ittaibaratz commented Nov 24, 2021

Fixes Issue: #16775

Our build system deploys iframe.html to a different folder than the main storybook HTML.

This is not a problem for our stories since we set PREVIEW_URL to the correct iframe.html location, however we recently added the docs add-on and found that when stories are embedded in docs (inlineStories = false), they do not honor the PREVIEW_URL but rather use "iframe.html" directly.

What I did

Default Story component base URL will now use PREVIEW URL if available, otherwise it will use "iframe.html" as it did before.
This approach is aligned with other places in the code which follow the same pattern to support alternative folders for the iframe html.

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 Nov 24, 2021

Nx Cloud Report

CI ran the following commands for commit fa16dcc. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@shilman
Copy link
Member

shilman commented Nov 25, 2021

Hi @ittaibaratz thanks for initiating this! However, I don't think it fixes the issue in the current state, and I'm not sure what's required to make it work. Documenting what I know here:

Storybook is divided into a manager UI (typically /index.html) and a preview iframe (typically /iframe.html). Passing the --preview-url into Storybook allows you to replace /iframe.html with an arbitrary URL. This is available in the manager as the global variable PREVIEW_URL here: https://github.com/storybookjs/storybook/blob/next/lib/manager-webpack4/src/presets/manager-preset.ts#L106

if the Story block, modified in this PR, were rendered in the manager, everything would be hunky dory. However, addon-docs is currently rendered in the preview, which means that its URL is the same as the story URL, but with different query parameters. Therefore this PR should probably be updated to use `window.location.

As a side note, I tried updating the PR to make PREVIEW_URL available in the preview (by updating iframe-webpack.config.ts), but when I tested it out with examples/standalone-preview per its README instructions, but it didn't work, so I'm a little confused there.

@ittaibaratz
Copy link
Contributor Author

Hi @ittaibaratz thanks for initiating this! However, I don't think it fixes the issue in the current state, and I'm not sure what's required to make it work. Documenting what I know here:

Storybook is divided into a manager UI (typically /index.html) and a preview iframe (typically /iframe.html). Passing the --preview-url into Storybook allows you to replace /iframe.html with an arbitrary URL. This is available in the manager as the global variable PREVIEW_URL here: https://github.com/storybookjs/storybook/blob/next/lib/manager-webpack4/src/presets/manager-preset.ts#L106

if the Story block, modified in this PR, were rendered in the manager, everything would be hunky dory. However, addon-docs is currently rendered in the preview, which means that its URL is the same as the story URL, but with different query parameters. Therefore this PR should probably be updated to use `window.location.

As a side note, I tried updating the PR to make PREVIEW_URL available in the preview (by updating iframe-webpack.config.ts), but when I tested it out with examples/standalone-preview per its README instructions, but it didn't work, so I'm a little confused there.

Thanks @shilman, it does work for us, but I realized its because we added this code that injects the PREVIEW_URL into the iframe's webpack config via main.js:

const updateIframePath = (config) => {
    const configHtmlWebPackPlugin = config.plugins.find((plugin) => plugin.constructor.name === 'HtmlWebpackPlugin');
    configHtmlWebPackPlugin.options.publicPath = config.output.publicPath;
    const { templateParameters: origTemplates } = configHtmlWebPackPlugin.options;
    configHtmlWebPackPlugin.options.templateParameters = (compilation, files, options) => {
        const params = origTemplates(compilation, files, options);
        return { ...params, globals: { ...params.globals, PREVIEW_URL } };
    };
};

...

    webpackFinal: async (config, { configType }) => {
        ...
        PREVIEW_URL && updateIframePath(config);
        ...

I wonder if I can contribute that back to Storybook somehow, I'll think about it...

@ittaibaratz
Copy link
Contributor Author

@shilman I've added PREVIEW_URL to the iframe webpack config. Will test and add unit tests after thanksgiving, but please let me know if you think this is the right direction, thank you!

@shilman
Copy link
Member

shilman commented Nov 26, 2021

Hi @ittaibaratz this is identical to the change I made to test it in examples/standalone-preview and I couldn't get it to work. It could be a problem with examples/standalone-preview, but the code looks correct to me. If you could test it on your end that would be really great.

@ittaibaratz
Copy link
Contributor Author

ittaibaratz commented Nov 26, 2021

@shilman I had issues getting the standalone example to run at all, I think its because yarn is not passing the --preview-url query parameter to the script, also parcel was complaining about browserslist config for me.

So I ended up making some minor tweaks to the standalone's example package json so it can run the official storybook directly. That allows me to run the parcel stories inside storybook. Now I'll try to get the docs add-on used in that example so I can show my code change actually works :)

As a side note, the example can be updated to use Parcel 2, we use it a lot here so I can easily update it it adds any value.

@shilman
Copy link
Member

shilman commented Nov 26, 2021

@ittaibaratz that would be really awesome if you could update to Parcel 2. we're not using that example heavily, but it does come up every so often and having it on the latest parcel version would be a real help 🙏

@ittaibaratz
Copy link
Contributor Author

@ittaibaratz that would be really awesome if you could update to Parcel 2. we're not using that example heavily, but it does come up every so often and having it on the latest parcel version would be a real help 🙏

done and done :) Please review and provide your comments. I had to bump Telejson to 5.3.3 for the Parcel 2 upgrade, otherwise it was pretty smooth.

@ittaibaratz
Copy link
Contributor Author

ittaibaratz commented Nov 26, 2021

@shilman , just one observation is that the way I've done it is by adding a PREVIEW_URL ENV to the Parcel build command then adding a tiny script that copies it to global (window).

It can also be done without the ENV by just copying location.href to global, since essentially we want the preview to point at itself. I wonder if that can be done directly in Story.tsx? Not familiar enough with the Storybook architecture.

Anyways doesn't look like this is an issue for many users so the way I've done it now is probably the safest and also provides the most flexibility.

@ittaibaratz
Copy link
Contributor Author

Hi @shilman , I wonder if you can help me figure out why the netlify job is not passing? I would like to get this PR merged as soon as possible as this is blocking our internal storybooks. Thank you!

@shilman
Copy link
Member

shilman commented Dec 6, 2021

Hey @ittaibaratz sorry to hear this is blocking you. I'll try to get it over the line today.

@shilman
Copy link
Member

shilman commented Dec 9, 2021

@ittaibaratz Sorry i got pulled away by something else. Will try again today. Thanks for your patience

@ittaibaratz
Copy link
Contributor Author

@ittaibaratz Sorry i got pulled away by something else. Will try again today. Thanks for your patience

Thank you @shilman , very much appreciated! I just merged the latest changes to this PR - Let me know if you have any concerns or find any issues.

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.

Looking good & works well on my machine. Thanks @ittaibaratz !!

@shilman shilman changed the title Support custom PREVIEW URL for block story iframe Core: Support custom PREVIEW URL for block story iframe Dec 9, 2021
@shilman shilman merged commit 9df10e9 into storybookjs:next Dec 9, 2021
@ittaibaratz
Copy link
Contributor Author

Yay! Thank you @shilman !

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

Successfully merging this pull request may close these issues.

2 participants