fix: improve typescript compilation

mihar-22 commented 3 years ago

Package: @custom-elements-manifest/analyzer

This PR fixes issue #57 and introduces a few improvements to the TypeScript compilation process.

Changes

Important to note ahead of the time that there are more changes in the git history than intended because simply saving some files caused prettier/eslint to reformat them. Main changes were simply inside the start of the create function inside create.ts and the new utils/compiler.ts file.

Moved typescript to peerDependencies to avoid version mismatch (see #57).
Use createProgram to compile instead of createSourceFile to add type checking. It'll fallback to using createSourceFile when path and sourceCode config option is present.
Add tsConfigName option to cem configuration file to enable users to specify a TS config file to use when compiling. If the option is not present it'll default to a sensible configuration file.
Pass program and typeChecker to plugins to enable deeper AST analysis or the ability to extract symbol/type information.
Updated README.md documentation with changes.

All tests are passing ✅

thepassle commented 3 years ago

Thanks for the quick work on this 🙂 There's a couple things I'd like to discuss:

Add tsConfigName option to cem configuration file to enable users to specify a TS config file to use when compiling. If the option is not present it'll default to a sensible configuration file.

Maybe we should try to find if a user already has a tsconfig in their project, and if they do; use that. If they dont, see if a ts config is specified in the config file, if not; default to our own config file?

Additionally; what happens if there are type errors during compilation? That will crash the analyzer right?

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?).

This makes sense, because typescript doesnt follow semver and can do breaking changes on minor versions (which seems to be case here, and imo is extremely annoying)

Regarding adding TS as a peerdependency, Im worried that this might cause some problems. Say a user uses typescript@^4.1.x, and they write a plugin locally in their project. Because this project uses ts@~4.0.0, they now have the mismatch between ts.SyntaxKinds. If they want to write a plugin locally, that means they have to install ts@~4.0.0, but that then might break their actual source code.

Even if users dont write plugins locally, but just want to use the analyzer as a CLI, they now have to install a specific version of TS which may not be compatible with their source code.

Im trying to think of a good solution here, but so far I'm coming up short 😕 Do you have any ideas here?

mihar-22 commented 3 years ago

Maybe we should try to find if a user already has a tsconfig in their project, and if they do; use that. If they dont, see if a ts config is specified in the config file, if not; default to our own config file?

I agree this might be a better approach and it is how I designed it at first but it started throwing errors when testing because in this library the tsconfig.json doesn't "include" the fixtures directory (understandably). I decided maybe it's best to use a default configuration and only find a config file when specified.

Potential solution: I can revert it back and try to get the tests working. Maybe a tsConfigPath: string is a better option than tsConfigName: string (monorepos could be a use-case). We first see if there's a custom config path via options, if not we look for tsconfig.json, if not we use sensible defaults? See next section.

Additionally; what happens if there are type errors during compilation? That will crash the analyzer right?

Unfortunately it will crash.

Potential solution: A tsTypeCheck: boolean option to make type checking and receiving program/typeChecker optional. This also means we don't have to search for a TS config file unless this option is true.

Regarding adding TS as a peerdependency, Im worried that this might cause some problems. Say a user uses typescript@^4.1.x, and they write a plugin locally in their project. Because this project uses ts@~4.0.0, they now have the mismatch between ts.SyntaxKinds. If they want to write a plugin locally, that means they have to install ts@~4.0.0, but that then might break their actual source code.

I'm not quite following here. If it's specified as a peer dependency then this library is free to use any version of TS within the peer dependency range (in this case I specified ^4.0.0) and install it as a dev dependency, AND the user is now unfortunately responsible for having to install TS but it means they can use whatever version they want as well (again within range). Am I missing something? I know it sucks that they'll have to install it but I'm not sure what else to do about it.

Maybe a post-install script that'll look if TS is already installed, if not install it?

thepassle commented 3 years ago

Closing this as per https://github.com/open-wc/custom-elements-manifest-deprecated/issues/57#issuecomment-855342356

open-wc / custom-elements-manifest-deprecated

fix: improve typescript compilation #58