open-wc / custom-elements-manifest

Custom Elements Manifest is a file format that describes custom elements in your project.
https://custom-elements-manifest.open-wc.org/
234 stars 43 forks source link

Private methods and properties with `_` prefix #39

Closed hsablonniere closed 3 years ago

hsablonniere commented 3 years ago

Hey team :wink:,

Context

I'm still using the _ prefix convention for private properties and methods. I know # is coming but I don't want to transpile and while I wait for better support, I'm OK with the _ prefix convention.

WCA recognize _ as a private prefix and it also recognize # as a private prefix I think.

If I use those _ prefix with CEM analyzer:

https://custom-elements-manifest.netlify.app/?source=ZXhwb3J0IGNsYXNzIE15Q29tcG9uZW50IGV4dGVuZHMgTGl0RWxlbWVudCB7CgogIHN0YXRpYyBnZXQgcHJvcGVydGllcyAoKSB7CiAgICByZXR1cm4gewogICAgICBfcHJpdmF0ZVByb3A6IHsgdHlwZTogU3RyaW5nIH0sCiAgICAgICNwcml2YXRlRm9vYmFyOiB7IHR5cGU6IFN0cmluZyB9LAogICAgfTsKICB9CgogIF9wcml2YXRlTWV0aG9kKCkgewogICAgICBjb25zb2xlLmxvZygnZm9vJyk7CiAgfQoKICAjcHJpdmF0ZU1ldGhvZCgpIHsKICAgICAgY29uc29sZS5sb2coJ2ZvbycpOwogIH0KfQoKd2luZG93LmN1c3RvbUVsZW1lbnRzLmRlZmluZSgnbXktY29tcG9uZW50JywgTXlDb21wb25lbnQpOw%3D%3D&library=litelement

Examples with /** @private */:

https://custom-elements-manifest.netlify.app/?source=ZXhwb3J0IGNsYXNzIE15Q29tcG9uZW50IGV4dGVuZHMgTGl0RWxlbWVudCB7CgogIHN0YXRpYyBnZXQgcHJvcGVydGllcyAoKSB7CiAgICByZXR1cm4gewogICAgICAvKiogQHByaXZhdGUgKi8KICAgICAgX3ByaXZhdGVQcm9wOiB7IHR5cGU6IFN0cmluZyB9LAogICAgICAvKiogQHByaXZhdGUgKi8KICAgICAgI3ByaXZhdGVGb29iYXI6IHsgdHlwZTogU3RyaW5nIH0sCiAgICB9OwogIH0KCiAgLyoqIEBwcml2YXRlICovCiAgX3ByaXZhdGVNZXRob2QoKSB7CiAgICAgIGNvbnNvbGUubG9nKCdmb28nKTsKICB9CgogIC8qKiBAcHJpdmF0ZSAqLwogICNwcml2YXRlTWV0aG9kKCkgewogICAgICBjb25zb2xlLmxvZygnZm9vJyk7CiAgfQp9Cgp3aW5kb3cuY3VzdG9tRWxlbWVudHMuZGVmaW5lKCdteS1jb21wb25lbnQnLCBNeUNvbXBvbmVudCk7&library=litelement

Expectations

  1. I expected _ and # to be handled as private.
  2. I expected not to have the private stuffs in the JSON output.

What do you think about those?

thepassle commented 3 years ago

While methods prefixed with _ being private is a widely used convention, it is still a convention, and methods prefixed with a _ are not actually private (unlike methods prefixed with a #), therefore I think its more correct to stick to the standard, and not mark these as private. As discussed on Slack, I recommend using a custom plugin to make them private instead. A quick example of a plugin that could do this could look like:

function privatizePlugin() {
  return {
    name: 'privatize-methods',
    packageLinkPhase({customElementsManifest, context}){
        customElementsManifest?.modules?.forEach(mod => {
            mod?.declarations?.forEach(dec => {
                if(dec.customElement) {
                    dec.members = dec?.members?.map(member => {
                        return member.name.startsWith('_') ? {...member, privacy: 'private'} : member;
                    })
                }
            });
        });
    },
  }
}

Regarding:

  static get properties () {
    return {
      /** @private */
      _privateProp: { type: String },
      /** @private */
      #privateFoobar: { type: String },
    };
  }

We indeed don't handle these yet, and we can (and should) improve on that.

daKmoR commented 3 years ago

interestingly for me _foo was "protected" and __foo was "private"...

I would say stick to the standard... but maybe add this plugin to the docs? or even ship the plugin as a package? or even add it to the "core" it so you could do

import { privatizePlugin } from '...';

export default {
  plugins: [privatizePlugin(fnName => fn.startsWith('__'))]
 }
thepassle commented 3 years ago

Did you have them annotated with jsdoc as being protected/private? Because by default we dont handle privacy based on the method name. Jsdoc could also be a solution here, then you dont even need a plugin to handle it (but it does require adding jsdoc to your code)

daKmoR commented 3 years ago

indeed we annotate with jsdoc... so if that works already... then I would say declaring it in TS or via JSDoc or via js # should be the only ways to do it... clean, descriptive, and no magic 💪