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

[jet-brains-integration] Include source module and symbol in web-types #94

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Kouzukii
Copy link

Thank you for making this awesome generator!

Web-Types allows for specifying source module and symbol.
When these are set you can use Go To > Declaration (or more commonly Ctrl+Click) on the custom HTML element to jump directly to the source file.
The info is already present in the CEM and just needs to be passed along.

(Tested with litelements)

@break-stuff
Copy link
Owner

The challenge I see with this is that the module path is usually inaccurate in the CEM since it references the source file and not the output. Since this source file is not usually included in the final package, this will be useless for most teams. I know there are tools out there to update this path, so I think this could be a valid feature, but I think we should avoid making it enabled by default. Instead, could we invert the property to something like inludeSource, so teams can opt into it if they have made the changes?

@break-stuff
Copy link
Owner

I wonder if it would be beneficial to provide an option for teams to supply their own source path in case they haven't updated the CEM with the new information.

@Kouzukii
Copy link
Author

Kouzukii commented Feb 27, 2024

We could allow the user to pass a function that provides the source object given a component and module.
By default this function returns (component, module) => ({ symbol: component.name, module: module.path })

@break-stuff
Copy link
Owner

break-stuff commented Mar 7, 2024

We could do something similar to what we have done for the other tools where we pass the tag name and the class name and they can define a path:

sourceModule: (className, tagName) => `../dist/components/${tagName}/${className}.js`

@veith
Copy link
Contributor

veith commented Mar 10, 2024

We could do something similar to what we have done for the other tools where we pass the tag name and the class name and they can define a path:

sourceModule: (className, tagName) => `../dist/components/${tagName}/${className}.js`

This is not so nice, because the path can vary, if you put some components in sub directories,..

@veith
Copy link
Contributor

veith commented Mar 10, 2024

We could allow the user to pass a function that provides the source object given a component and module. By default this function returns (component, module) => ({ symbol: component.name, module: module.path })

@Kouzukii Just to get it right. The return value is a object literal?

Because web-types.json accepts also {file,offset} for the source attribute.

/**
 * Allows to specify the source of the entity. For Vue.js component this may be for instance a class.
 */
export type Source =
  | {
      /**
       * Path to the file, relative to the web-types JSON.
       */
      file: string;
      /**
       * Offset in the file under which the source symbol, like class name, is located.
       */
      offset: number;
    }
  | {
      /**
       * Name of module, which exports the symbol. May be omitted, in which case it's assumed to be the name of the library.
       */
      module?: string;
      /**
       * Name of the exported symbol.
       */
      symbol: string;
    }

https://raw.githubusercontent.com/JetBrains/web-types/master/schema/web-types.json

"source": {
      "description": "Allows to specify the source of the entity. For Vue.js component this may be for instance a class.",
      "type": "object",
      "oneOf": [
        {
          "type": "object",
          "properties": {
            "file": {
              "description": "Path to the file, relative to the web-types JSON.",
              "type": "string"
            },
            "offset": {
              "description": "Offset in the file under which the source symbol, like class name, is located.",
              "type": "integer"
            }
          },
          "required": [
            "file",
            "offset"
          ],
          "additionalProperties": false
        },
        {
          "type": "object",
          "properties": {
            "module": {
              "description": "Name of module, which exports the symbol. May be omitted, in which case it's assumed to be the name of the library.",
              "type": "string"
            },
            "symbol": {
              "description": "Name of the exported symbol.",
              "type": "string"
            }
          },
          "required": [
            "symbol"
          ],
          "additionalProperties": false
        }
      ]
    }

@Kouzukii
Copy link
Author

@Kouzukii Just to get it right. The return value is a object literal?

Because web-types.json accepts also {file,offset} for the source attribute.

Yeah, I think allowing the user to pass the full object is the best way to go.
I haven't tried file/offset, since module/symbol already worked in my usecase.

@break-stuff
Copy link
Owner

break-stuff commented Mar 10, 2024

We could do something similar to what we have done for the other tools where we pass the tag name and the class name and they can define a path:

sourceModule: (className, tagName) => `../dist/components/${tagName}/${className}.js`

This is not so nice, because the path can vary, if you put some components in sub directories,..

We have those too. That string returned by the function is completely configurable by the developer and can return whatever they would like:

sourceModule: (className, tagName) => {
  switch(tagName) {
    case 'tab':
    case 'tabs':
    case 'tab-panel':
      return `../dist/components/tabs/${className}.js`;
    case 'accordion':
    case 'accordion-panel':
      return `../dist/components/accordion/${className}.js`;
    default:
      return `../dist/components/${tagName}/${className}.js`;
  }
}

@break-stuff
Copy link
Owner

break-stuff commented Mar 10, 2024

We could allow the user to pass a function that provides the source object given a component and module. By default this function returns (component, module) => ({ symbol: component.name, module: module.path })

@Kouzukii Just to get it right. The return value is a object literal?

Because web-types.json accepts also {file,offset} for the source attribute.

Are you typically defining more than one custom element per module or file?

@veith
Copy link
Contributor

veith commented Mar 11, 2024

We could allow the user to pass a function that provides the source object given a component and module. By default this function returns (component, module) => ({ symbol: component.name, module: module.path })

@Kouzukii Just to get it right. The return value is a object literal?
Because web-types.json accepts also {file,offset} for the source attribute.

Are you typically defining more than one custom element per module or file?

Usually not, but in our analyzer config we check for every declaration in the declarations block , just to be on the save side.

@veith
Copy link
Contributor

veith commented Mar 11, 2024

Source

IMHO: If you write a "rule" you don't have to touch the config file again.

In our company, the one who maintains the "build" stuff is not the dev.

@break-stuff
The {className, tagName} as arguments will work for me too, with that information I am able to load the cem file and filter out the needed data.

BTW: The return type should not be a simple string, i think it should be more like:
{symbol:string,module?:string} | {file:string, offset:number} | false .

The false should be used to decide to not set the sourceModule for an element at all (i.e. if it is deprecated or for other reasons).

@break-stuff
Copy link
Owner

I think all we need is the file path for the source module.

We already know the name, so that doesn't need to be provided.

From the projects I have worked on, I have yet to see a team put multiple components in the same file, so the offset property doesn't seem especially useful for web components.

I don't think the {symbol:string,module?:string} option really makes sense for web components either. Do you have a good scenario for this use-case?

@Kouzukii
Copy link
Author

@break-stuff I've added a sourceModuleTemplate option now to allow the user to customize the generated source module path (also fixed referenceTemplate since it wasn't being set)
What do you think?

@@ -20,6 +20,10 @@ export interface Component extends schema.CustomElementDeclaration {
};
}

export interface ComponentWithModule extends Component {
module: CustomModule;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. There shouldn't be an array of components in a component.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the WebTypeElement type needs to be updated instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of the module here is necessary to pass module.path along. But you're right WebTypeElement also needs to be adjusted.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomModule has a property called declarations that contains an array of Components.

@@ -51,17 +51,18 @@ export function getComponentDescription(
export function getComponents(
customElementsManifest: CEM,
exclude?: string[]
): Component[] {
): ComponentWithModule[] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be reverted.

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

Successfully merging this pull request may close these issues.

3 participants