open-wc / custom-elements-manifest-deprecated

Custom Elements Manifest is a file format that describes custom elements in your project.
8 stars 4 forks source link

Analyzer - Invalid Syntax Kind #57

Closed mihar-22 closed 3 years ago

mihar-22 commented 3 years ago

Hey @thepassle!

First off thanks heaps for the hard work on this library. I'm having some slight issues trying to extend the manifest in the analyzePhase within my custom plugin. All the syntax kinds are messed up for some reason, classes are returning as variable declarations, enums as class declarations, types as functions and so on. All the code is available here, you can also see the plugin and what not inside the cem directory (nothing special).

Another issue closely related is I'm trying to get "inherited" documentation on a property or method from an interface but it currently doesn't work. Sometimes it's preferable to write an interface and implement it across different components. Documenting solely the single interface saves energy in avoiding to have to document multiple component props/methods over and over.

Normally the recommended way to get documentation is to retrieve it on a symbol via the TypeChecker. Something like...

// EXAMPLE
const comment = checker.getSymbolAtLocation(identifier)?.getDocumentationComment(checker);
return normalizeLineBreaks(ts.displayPartsToString(comment) ?? '');

This library currently doesn't parse files using createProgram (probably performance reasons to avoid type checks?) and instead uses createSourceFile, unfortunately this loses type information which can be used in the analyzePhase by a custom plugin. In my case this could be used to get better type inference and symbol documentation.

Is this something we can resolve in this library or should I fork?

thepassle commented 3 years ago

Hi @mihar-22 !

Thats weird. Plugins are passed the AST nodes as they're being iterated through here, I've not seen this problem before myself when writing plugins. I'll take a look at your repo and investigate.

This library currently doesn't parse files using createProgram (probably performance reasons to avoid type checks?) and instead uses createSourceFile, unfortunately this loses type information which can be used in the analyzePhase by a custom plugin. In my case this could be used to get better type inference and symbol documentation.

Is this something we can resolve in this library or should I fork?

Are the two interchangeable? Does using createSourceFile return a different than createProgram? If they are interchangeable and createProgram gives more information I'd be willing to accept a PR for it, but if it drastically changes the AST then that might not be possible, since pretty much everything depends on the AST looking the way it does.

mihar-22 commented 3 years ago

Ye I was confused as well... I can't really see any reason for the syntax kind being messed up.

It doesn't change the AST structure. The createProgram function will also perform type checking which means slower compilation (I don't know by how much). Once you have a Program you can get symbols/type information using the TypeChecker via program.getTypeChecker(). Maybe this can be an optional feature that you opt in for via the configuartion file?

Reference: https://github.com/microsoft/TypeScript-wiki/blob/master/Using-the-Compiler-API.md

mihar-22 commented 3 years ago

Figured out what's wrong, it's a simple TypeScript version mismatch. This library is locked to TypeScript version 4.0.3 and my library is using 4.1.3. It seems like the ts.SyntaxKind has changed between those versions so that's why nothing was correct (weird because wouldn't this be a breaking change?). For example, in one ts.SyntaxKind.ClassDeclaration is 252 and in the other it's 249.

I think my recommended solution would be let typescript be a required peer dependency, and in development install it as a dev dependency. I'll submit a PR with also the added change of using createProgram so you can review 😄

thepassle commented 3 years ago

Sorry for the delay on this, it took quite a bit of thinking and work to see what the best approach here would be. This should be resolved in the new version of the analyzer.

In the new analyzer you can specify a overrideModuleCreation property in your custom-elements-manifest.config.js like so:

import { defaultCompilerOptions } from './compilerOptions.js';
import { myPlugin } from './my-plugin.js';

let typeChecker;

export default {
  overrideModuleCreation: ({ts, globs}) => {
    const program = ts.createProgram(globs, defaultCompilerOptions);
    typeChecker = program.getTypeChecker();

    return program.getSourceFiles().filter(sf => globs.find(glob => sf.fileName.includes(glob)));
  },
  plugins: [
    /** You can now pass the typeChecker to your plugins */
    myPlugin(typeChecker)
  ],
}

To give users a bit more freedom in how modules are created and if they want to use the typechecker (which by itself is not used in the analyzer).

As for the TS version mismatch, the analyzers TS instance is now passed to plugin hooks as an argument, so users don't have to add typescript as a peer dependency, and can be sure to use the same version of TS as the analyzer to avoid mismatches.

For more information, see https://github.com/open-wc/custom-elements-manifest/tree/master/packages/analyzer#overriding-sourcefile-creation