Ignore private members and methods from Custom Elements Manifest

[edoardocavazza]

Describe the bug The new setCustomElementsManifest method correctly loads a Custom Element Manifest and it populates properties in the API table, but it does not filter private members using the privacy field in the manifest.

To Reproduce https://github.com/edoardocavazza/custom-elements-manifest-private-members

Run the storybook and open docs page.

System

Environment Info:

  System:
    OS: macOS 11.3
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
  Binaries:
    Node: 15.14.0 - ~/.nvm/versions/node/v15.14.0/bin/node
    Yarn: 2.4.2 - /usr/local/bin/yarn
    npm: 7.7.6 - ~/.nvm/versions/node/v15.14.0/bin/npm
  Browsers:
    Edge: 91.0.864.48
    Firefox: 89.0
    Safari: 14.1
  npmPackages:
    @storybook/addon-actions: ^6.4.0-alpha.9 => 6.4.0-alpha.9 
    @storybook/addon-docs: ^6.4.0-alpha.9 => 6.4.0-alpha.9 
    @storybook/addon-essentials: ^6.4.0-alpha.9 => 6.4.0-alpha.9 
    @storybook/addon-links: ^6.4.0-alpha.9 => 6.4.0-alpha.9 
    @storybook/web-components: ^6.4.0-alpha.9 => 6.4.0-alpha.9

[thepassle]

It probably only should filter out fields and method that are actually private (e.g.: prefixed with a #). Developers may use JSDoc to annotate "private" (or: "pseudo-private") properties and method, when in fact they are not enforced by the language itself. As a developer, while developing, I'd still want to be able to tweak "pseudo-private" properties.

I do agree it doesnt make sense to display truly private properties, because that'll just give an error.

We should probably add a check here: https://github.com/storybookjs/storybook/blob/5bd7e02c841ce79e41664a999dd7c50290859c63/addons/docs/src/frameworks/web-components/custom-elements.ts#L55

And add something like:

if(acc?.name?.startsWith('#') return

[edoardocavazza]

I see. Perhaps the kind of filter I suggested can be moved to the analyzer with an optional flag? Something similar to the web-component-analyzer visibility option.

[thepassle]

I think its unlikely I'll add something like that to the core analyzer, but it should be easy enough to do with a custom plugin: https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/docs/plugins.md

[dschwank]

Excluding the private fields in the visualization would a good idea from my point of view. 👍

As a developer, while developing, I'd still want to be able to tweak "pseudo-private" properties.

I think we have to differentiate here a little bit - some devs might need it, but as also non-devs are using storybook, it could be weird to see "pseudo-private" fields. As many frameworks are based on decorators, I am not sure if it would be possible to enforce # for all of them - even though that would be the best.

@thepassle & @edoardocavazza What do you think about excluding the private fields and make it optional / configurable to only show the pseudo-private fields?

[thepassle]

@thepassle & @edoardocavazza What do you think about excluding the private fields and make it optional / configurable to only show the pseudo-private fields?

It should be done on the level of the CEM, not in Storybook. If a developer sets up their storybook for non-devs, they should make sure their CEM includes the things that are relevant for non-devs. The CEM is just a JSON object and can easily be manipulated via scripts, or a custom plugin.

[dschwank]

Thanks for the input @thepassle !

It should be done on the level of the CEM, not in Storybook.

Hmm - I am not really convinced yet. From my point of view one should not touch private fields, even if they are pseudo-private. Doing it on the CEM level makes it less standardized. Also the viewer provided by the open-wc is also just showing the public fields: https://github.com/open-wc/api-viewer-element/blob/master/packages/api-docs/src/base.ts#L30

Or am I missing something?

Could you share some more thoughts on this?

[thepassle]

From my point of view one should not touch private fields, even if they are pseudo-private.

I use Storybook at work for local development, and having the pseudo-private fields are definitely useful to me in my daily work.

Also the viewer provided by the open-wc is also just showing the public fields: https://github.com/open-wc/api-viewer-element/blob/master/packages/api-docs/src/base.ts#L30

I wasnt aware of this — I'm of the opinion api-viewer should definitely also list pseudo-private fields, I'll contact Serhii about this.

Could you share some more thoughts on this?

I still think outputting a separate CEM for your dedicated/published Storybook would an easy way to solve this issue, it's also very straightforward to filter out anything you need via plugins, and a package.json script.

However, thinking more on it, we could indeed add a configuration object to the setCustomElementsManifest function, e.g.:

import cem from './custom-elements.json' assert { type: 'json' };

setCustomElementsManifest(cem, { privateFields: false });

[WillGeller]

@thepassle My team would greatly appreciate a privateFields: false option on setCustomElementsManifest, we hold a similar viewpoint to @dschwank and feel that it is valuable for the CEM to have all the metadata reflective of the web components internals, but it would be great to have Storybook give us the option to hide @state() private type of properties from the Storybook docs, which is largely used to tell component library consumers the input/output API of the components, not it's underlying implimentation.

[dschwank]

Thanks for sharing @thepassle & @WillGeller !

So let's try to go with a privateFields option. Has anyone started with the implementation already? If not, then I would make a first draft where we could continue the discussion. ✌️

[nedredmond]

import cem from './custom-elements.json' assert { type: 'json' };

setCustomElementsManifest(cem, { privateFields: false });

I just implemented this in a pretty ugly wrapper function to filter out undesirable docs:

import { setCustomElementsManifest } from '@storybook/web-components';

export const setCustomElementsManifestWithOptions = (
  customElements: any,
  options: { privateFields?: boolean },
): void => {
  let { privateFields = true } = options;
  if (!privateFields) {
    customElements?.modules?.forEach((module: { declarations: any[] }) => {
      module?.declarations?.forEach(declaration => {
        Object.keys(declaration).forEach(key => {
          if (Array.isArray(declaration[key])) {
            declaration[key] = declaration[key].filter(
              (member: { privacy: string | string[] }) =>
                !member.privacy?.includes('private'),
            );
          }
        });
      });
    });
  }
  return setCustomElementsManifest(customElements);
};

[sebastian-kreft-simployer]

It probably only should filter out fields and method that are actually private (e.g.: prefixed with a #). Developers may use JSDoc to annotate "private" (or: "pseudo-private") properties and method, when in fact they are not enforced by the language itself. As a developer, while developing, I'd still want to be able to tweak "pseudo-private" properties.

I do agree it doesnt make sense to display truly private properties, because that'll just give an error.

We should probably add a check here:

https://github.com/storybookjs/storybook/blob/5bd7e02c841ce79e41664a999dd7c50290859c63/addons/docs/src/frameworks/web-components/custom-elements.ts#L55

And add something like:

if(acc?.name?.startsWith('#') return

Is there any progress on it? This mapper ignores ALL the methods, but I need to document some methods in some components, because they are "truely" public (they are designed to be used by developers).

[fdendorfer]

In case someone else needs this, here's my simplified, but probably imperfect, solution:

// custom-elements-manifest.config.mjs
export default {
  // ...
  plugins: [
    // ...

    // Filter private fields
    {
      name: 'web-components-private-fields-filter',
      analyzePhase({ ts, node, moduleDoc }) {
        switch (node.kind) {
          case ts.SyntaxKind.ClassDeclaration: {
            const className = node.name.getText();
            const classDoc = moduleDoc?.declarations?.find((declaration) => declaration.name === className);

            if (classDoc?.members) {
              classDoc.members = classDoc.members.filter((member) => !member.privacy);
            }
          }
        }
      },
    },
  ],
};