Ensure Angular docsMode flag has effect

[alexandertrefz]

What I did

Fix what #20608 was setting out to do: Fixing docs generation for Angular.

#20608 fixed the issue that the angular builders accept a docsMode flag, but were internally attempting to read a flag called docs to pass that along to runInstance, thereby always reading undefined and passing it along.

#20608 fixed this successfully, but introduced a new bug: It now passes along the docsMode flag, reading the value correctly, but runInstance doesn't accept this flag, it looks for a flag called docs.
It accepts a type called StandaloneOptions, which is a combination of CLIOptions and some other types, CLIOptions has the optional property docs as can be verified here: https://github.com/storybookjs/storybook/blob/next/code/lib/types/src/modules/core-common.ts#L159

I now simply read the docsMode flag but pass this along as docs. This fixes the generation successfully.

[alexandertrefz]

/cc @ndelangen

[valentinpalkovic]

@tmeasday, @JReinhold You have more knowledge about this. What does the error mean?

https://app.circleci.com/pipelines/github/storybookjs/storybook/39103/workflows/859db4a7-a376-41aa-b174-2651f8234a45/jobs/473122/steps?invite=true#step-104-32

And:
https://app.circleci.com/pipelines/github/storybookjs/storybook/39103/workflows/859db4a7-a376-41aa-b174-2651f8234a45/jobs/473122/steps

[JReinhold]

Hmm it means that there is no Story Indexer that can handle *.stories.mdx files, even though they are in the glob. Maybe we removed the indexer to handle story definitions in MDX, and forgot to remove this? I'm unsure, and I don't know if it is related to this PR or not.

@tmeasday or @shilman would know more.

[alexandertrefz]

This PR changes 2 lines, in passing along a single argument to reenable docs generation for Angular. It literally just renames a single key in an object (in 2 separate files) - I could not imagine it possibly impacting Story indexing in any way.

[sheriffMoose]

Hey @valentinpalkovic, @JReinhold
currently the common preset indexer works with .js|.jsx|.ts|.tsx and the indexing for .mdx resides in the addon-docs

[tmeasday]

Y'all have this backwards I suspect.

The CLI option is --docs. It should set the config option docs.docsMode = true.

I think what this is doing is setting docs = <value of cli flag>, so when the CLI flag is unset, it is setting docs = false | null | whatever, which has the effect of disabling docs. So now .stories.mdx files are not being indexed, as that indexer is added by the docs addon.

[ssams]

#20608 fixed this successfully, but introduced a new bug: It now passes along the docsMode flag, reading the value correctly, but runInstance doesn't accept this flag, it looks for a flag called docs. It accepts a type called StandaloneOptions, which is a combination of CLIOptions and some other types, CLIOptions has the optional property docs as can be verified here: https://github.com/storybookjs/storybook/blob/next/code/lib/types/src/modules/core-common.ts#L159

interesting, when I created the original PR it certainly worked with the latest v7/next at that time (the patch from the linked PR for sure works with current v6 releases, as I'm using that locally). Something must have changed internally since then?
Note that in addition to the CLIOption you've linked, which is called indeed just docs:

storybook/code/lib/types/src/modules/core-common.ts

Line 159 in 8073d04

docs?: boolean;

There's also another docsMode in the BuilderOptions type just a few lines later in the file:

storybook/code/lib/types/src/modules/core-common.ts

Line 170 in 8073d04

docsMode?: boolean;

I'd assume it worked so far as it directly set the latter one and it got forwarded / kept included in the config option until reaching the actual builder, could be that types have been improved or gotten stricter in the meantime and it's only now filtered out somewhere in the call chain.

Also at the end of the Angular builder buildStaticStandalone is called with the collected options. This function apparently accepts a combination of CLIOptions and BuilderOptions, so from the type alone both docs or docsMode would be valid to pass (but apparently they don't work equally in practice):

storybook/code/frameworks/angular/src/builders/build-storybook/index.ts

Line 125 in 8073d04

() => buildStaticStandalone(options)

storybook/code/lib/core-server/src/build-static.ts

Lines 37 to 41 in 8073d04

	export type BuildStaticStandaloneOptions = CLIOptions &
	LoadOptions &
	BuilderOptions & { outputDir: string };
	export async function buildStaticStandalone(options: BuildStaticStandaloneOptions) {

[alexandertrefz]

Y'all have this backwards I suspect.

The CLI option is --docs. It should set the config option docs.docsMode = true.

I think what this is doing is setting docs = <value of cli flag>, so when the CLI flag is unset, it is setting docs = false | null | whatever, which has the effect of disabling docs. So now .stories.mdx files are not being indexed, as that indexer is added by the docs addon.

This was a good hint!

I know for certain that we need to set the docs flag for the docsMode to kick in. But you were right that my change does indeed seem to turn off the addon-docs in its entirety when docsMode is not set. I was able to reproduce the behaviour locally.

I have updated the PR to only set docs when docsMode is truthy, which has remedied the situation, at least locally.

[alexandertrefz]

Some fighting with the linter later, and it all seems to run through green. 🎉

I guess someone needs to approve the other 2 workflows?

[tmeasday]

Ok, my apologies there was more going on here than I realised.

So this is pretty confusing. There is:

• CLIOptions['docs']
• BuilderOptions['docsMode'] - which is directly set via the above.

Then there is:

• StandaloneOptions = CLIOptions & BuilderOptions -- which means it has both fields I guess (and they mean the same thing?)

(The above is angular, but there's also a core-server version of that)

Additionally, there is:

• StorybookOptions['docs'], which is an object, with a key docsMode.

I guess my suspicion about the indexer was wrong then? It seems an awful coincidence. Or could it be that options.presets.apply('docs') looks at options.docs somehow?

[alexandertrefz]

Then there is:

• StandaloneOptions = CLIOptions & BuilderOptions -- which means it has both fields I guess (and they mean the same thing?)

Actually no! They don't do the same thing. docsMode has been set since the original PR from last week (#20608) but it does not turn on --docs properly. Only passing docs to StandaloneOptions does turn on --docs. However, passing a falsy value to docs explicitly creates the MDX issues we observed in the beginning of this PR (probably turning off addon-docs entirely). Leaving docs out of the options entirely if it is falsy seems to create the default behaviour for the non-angular CLI.

I am unsure if docsMode has any effect at this point, but im not familiar enough with the codebase to make a determination either way.

[alexandertrefz]

@tmeasday So can this get merged, or do you want to investigate deeper?

[tmeasday]

@alexandertrefz I think the reason for that (needing to set options.docs = true to turn on docs mode) is due to this line:

https://github.com/storybookjs/storybook/blob/2cceaadef83bcc109af4144030a5e79af1a9d48f/code/lib/core-server/src/presets/common-preset.ts#L207-L214

The second argument to docs() there (a preset property) is really the whole Options object that includes both .docs and .docsMode but we only read options.docs when setting the default 'docs.docsMode' preset property.

That preset property is then read by:

• The index generator setup code
• The various builders (manager, webpack, vite).

I can't see any other places that either options.docs or options.docsMode is read 🤷

[alexandertrefz]

@tmeasday So what should be the course of action? Should we attempt to change the common-preset to look at both docs and docsMode then?

[tmeasday]

OK, I tracked it down. The problem is the essentials addon drops the docs addon if PresetOptions['docs'] is false here:

storybook/code/addons/essentials/src/index.ts

Line 51 in 1c7d2fa

.filter((key) => (options as any)[key] !== false)

So what's happening is that when we set options.docs = false it triggers the above. Setting it true works fine (thus this PR's ...(docsMode ? { docs: docsMode } : {}) works).

This simulates what SB proper does where the CLI option is left unset if you don't pass --docs (so you don't end up with options.docs = false).

Setting options.docsMode does nothing I'm pretty sure (right now) and probably would just confuse things on this PR.

However, this is all a big mess. @shilman we should discuss this.

[tmeasday]

Ok, discussed with @shilman.

It's pretty weird that we create that mess of StandaloneOptions (which can include both docs and docsMode booleans) and then combine it with the preset's actual options (in this case the options of the essential addon, which has a another, different option called docs). However, it would be a breaking change to do anything about the latter, and we are kind of stuck on the former as the CLI flag is called --docs also and we don't want to change it.

So the TLDR is .. it's super confusing, but the main thing is we do not ever want to set StandaloneOptions['docs'] to false. We should probably drop the unused docsMode boolean because it is just muddying the waters (and doesn't do anything AFAICT), but it doesn't need to happen in this PR. You could drop it from the angular side if you wanted.

So we can merge this PR as is or drop the docsMode, line. Or if we want to "fix" things I suggest we should drop docsMode everywhere and just rely on the option called docs. (i.e. delete this line and this line)

[ndelangen]

@alexandertrefz great work, could you apply the changes @tmeasday is asking about? then it's good to merge!

[alexandertrefz]

@tmeasday should we then also rename the angular flag to align with the rest and name it docs? So here renaming docsMode to docs and the same here?

So that docsMode is entirely gone?

(The PR is open to edits from you guys, if you just want to change those few lines yourselves, BTW)

[alexandertrefz]

I have added the requested changes and renamed the docsMode flag in the angular builders to docs - this should probably also be updated in the documentation for v7 Angular? I have updated a reference I found in the getting started guide, but I am unsure where the proper docs are located for this.

[alexandertrefz]

@ndelangen Just needs approval for the final workflows now and then it can get merged!

[ndelangen]

Invited you to the org @alexandertrefz, will merge as soon as CI is green. Awesome work, thank you for contributing!

[tmeasday]

Thank you for working through this with us!