Docs addon eval breaking Storybook Composition for certain prop default values

[anicholls]

I've discovered an issue with the Docs addon while attempting to set up my team's Storybook for Storybook Composition. This issue is always at play, it just doesn't become immediately apparent until you try to compose the Storybook.

React Docgen produces default values as strings, so @shilman added an eval to extractArgTypes in #10812. This works well most of the time, however it produces undesirable behavior when the default value is a string that matches something that exists on the window object.

In my case, I was trying to compose a Storybook with a Tooltip component that has a placement prop with the default 'top'. Since window.top provides a reference to the topmost window in the window hierarchy, this makes the Docs addon set the arg default to window. Try it yourself - run eval('top') in the console. Within that Storybook I haven't noticed any issues, however when you try to compose it in another Storybook, you get an error when the composing storybook tries to serialize the window object from another origin: Uncaught DOMException: Blocked a frame with origin from accessing a cross-origin frame.. You can see more info about my exploration of this issue in this comment. I opened a new issue because the one I linked originally relates to the actions addon (whereas this is an issue with docs).

Unfortunately, disabling the Docs addon for a specific story or set of stories is not an option, as the arg types seem to be extracted for every story regardless of this. The only way I can get Storybook Composition to work is by disabling the Docs addon entirely or by changing the default value of our component's placement prop (a breaking change that we would like to avoid).

If I wrap the try/catch in a conditional, everything works as expected:

let defaultValue = defaultSummary && (defaultSummary.detail || defaultSummary.summary);

if (!(defaultValue in window)) {
  try {
    defaultValue = eval(defaultValue);
  } catch (_unused) {}
}

This is completely blocking us from using Composition. I'd be willing to submit a PR with some guidance, however I'm not sure if the above snippet is the best approach. Alternatively, we could use JSON.parse instead of eval, but it depends what other inputs you expect (there are no tests to reference that I can find). Would love some feedback! Thanks 🙂

---

To Reproduce
Steps to reproduce the behavior:

1. Set up a storybook with @storybook/addon-docs enabled
2. Add a story for a component with a prop that has a default value of a string that exists on the window object (e.g. 'top', 'confirm', etc.)
3. Compose that storybook in another storybook
4. Inspecting the arg types generated for the story, you will see that the default for that prop is window.
5. Get the Uncaught DOMException: Blocked a frame with origin "http://localhost:9009" from accessing a cross-origin frame. error when loading that story in a composing Storybook.

Expected behavior
The default for the arg should be interpreted as expected (a string). Serializing stories across origins (for composition) should work as intended.

[shilman]

I believe this was fixed in #13919

Closing. Please comment if you're still seeing the issue in 6.2-beta! 🙏

[anicholls]

@shilman Thank you! Much appreciated.

I have tried to upgrade to 6.2.0-beta.11 to test and I am having babel issues. On start up I’m getting a bunch of Syntax Error: Missing class properties transform warnings. However, it looks like @babel/plugin-proposal-class-properties is still included in the default storybook babel plugins so I don't know why this is appearing all of a sudden. Even if I explicitly add it myself in main.js I get the same error. I saw a couple babel changes in the changelog, but nothing specifically related to this. Do you have any recommendations? or should I open another issue for the babel problem? Thanks in advance

[shilman]

@anicholls I think there's already an issue for this, so please look for it or open a new one and I can de-dupe. Thanks!!

[themagickoala]

I'm on Storybook 6.2 in two separate repositories, composing one (port 9009) into another (port 6006). I get the CORS error Uncaught DOMException: Blocked a frame with origin "http://localhost:9009" from accessing a cross-origin frame. at JSON.stringify, so I'm not sure this is fixed.

[anicholls]

@shilman I was finally able to upgrade to 6.2.9 after using resolutions and doing some dependency finageling. Now that I was able to test this out, I discovered that the issue is not fixed. This needs to be reopened.

The fix in #13919 was to use the following logic:

var defaultValueString = defaultSummary && (defaultSummary.detail || defaultSummary.summary);

try {
  if (defaultValueString) {
    // eslint-disable-next-line no-new-func
    defaultValue = Function("\"use strict\";return (".concat(defaultValueString, ")"))();
  } // eslint-disable-next-line no-empty

} catch (_unused) {}

This does not solve the original use case I provided. Try running Function("\"use strict\";return (".concat('top', ")"))() (note 'top' for the defaultValueString). It still returns the window object. This is still preventing us from using composition, close to a year after it was released 🙁 . Could you use JSON.parse for this or something similar? Let me know how we can get this fixed. Thanks!

[shilman]

Should be fixed in 6.3 by #14579

[tmeasday]

@anicholls thanks for looking at this so closely.

Just to be clear the fix in #13919 was more than just changing how we eval to be slightly safer. If something has fallen through the cracks, there may be a workaround besides the more nuclear #14579 which is coming in 6.3.

What the problem was:

1. react-docgen-typescript was returning incorrectly serialized values for some string defaults. It should have been returning the string "'top'" in your case, instead it returned the string "top" (no quotes).
2. We eval, which is pretty dodgy, but shouldn't be a problem if the things being eval-ed are properly serialized.

In #13919 I put some code in to workaround 1., as well as getting rid of 2 in some cases (but not all, yet, see #14579). I guess my workarounds are failing in your case. Can you give a bit more detail the exact setup for your component with a default value "top"? Specifically:

1. Are you using TS?
2. What is the prop type / TS type of the component's prop?
3. How is the default value specified?
4. What docgen loader are you using?

Thanks.

[anicholls]

@tmeasday Thanks for the quick response! Re. #14579, initializing optional default values to undefined would likely solve this issue, but I feel like that would introduce inaccuracies in the docs generated if I understand how it works correctly.

For our use case: you can have a closer look at the component/file in question here, but I'll share the details to answer your questions below:

Are you using TS?

Yes

What is the prop type / TS type of the component's prop?

export interface TooltipProps extends Omit<React.HTMLAttributes<HTMLDivElement>, 'title'> {
  // ...
  /**
   * Sets the placement preference used by PopperJS.
   * @default 'top'
   */
  placement?: Placement;
  // ...
}

Note: we have tried both 'top' and top in the JSDoc here, with the same results.

How is the default value specified?

It is initialized in the prop destructure:

export const Tooltip = ({
  type = 'label',
  placement = 'top',
  title,
  children,
  ...elemProps
}: TooltipProps) => {

What docgen loader are you using?

react-docgen-typescript

Thanks!

[tmeasday]

Hi @anicholls

Thanks for the details. There was one crucial thing I needed which was the exact type of Placement, but I think maybe it is coming from popper?:

https://github.com/popperjs/popper-core/blob/4800b37c5b4cabb711ac1d904664a70271487c4b/src/enums.js

In which case it is a union of strings?

In theory then that should work. It's complicated because there are a lot of different potentially string valued types that we need to "repair" when RDT gives us the wrong value, and it isn't always possible to actually do that. For instance, suppose you have this type:

type Pathological = "true" | true;

Then when RDT gives us the string "true" as the default value of the prop, what should we do with that? How should it be interpreted? This kind of madness is a big motivator behind #14579 as you might imagine.

In any case, we did encounter the case of a union of string types and added this code to deal with it:

storybook/addons/docs/src/lib/docgen/createPropDef.ts

Lines 27 to 45 in f3744e2

	function isStringValued(type?: DocgenType) {
	if (!type) {
	return false;
	}
	if (type.name === 'string') {
	return true;
	}
	if (type.name === 'enum') {
	return (
	Array.isArray(type.value) &&
	type.value.every(
	({ value: tv }) => typeof tv === 'string' && tv[0] === '"' && tv[tv.length - 1] === '"'
	)
	);
	}
	return false;
	}

It seems that's not working in this case. Perhaps you could investigate why? Or put a simple repro together?

There's also just the option of overriding the argType.defaultValue in this case, if you want to bail out on this whole problem ;)

Re. #14579, initializing optional default values to undefined would likely solve this issue, but I feel like that would introduce inaccuracies in the docs generated if I understand how it works correctly.

We are still going to use the inferred defaultValue string to write down the default values in the table, but it's less crucial there that we get it exactly right. For actual args that are passed into actual components, we are just going to let the framework do its thing when we don't pass any value in the prop. That's what default values are for after all.