github / catalyst

Catalyst is a set of patterns and techniques for developing components within a complex application.
https://github.github.io/catalyst/
MIT License
1.34k stars 50 forks source link

Build fail using vite vanilla-ts template #273

Closed jeromebon closed 1 year ago

jeromebon commented 2 years ago

I believe I followed all the steps in https://github.github.io/catalyst/guide/you-will-need/ (create a ts project and add experimentalDecorators) yet I get an error when I try to build a new project.

typescript version: 4.7.4 vite version: 3.0.8 catalyst version: 1.6.0

Steps to reproduce

1) Create a new project

npm create vite@latest Pick vanilla-ts

2) Add experimentalDecorators to tsconfig.json. The full tsconfig look like this:

{
  "compilerOptions": {
    "target": "ESNext",
    "useDefineForClassFields": true,
    "module": "ESNext",
    "lib": ["ESNext", "DOM"],
    "moduleResolution": "Node",
    "strict": true,
    "sourceMap": true,
    "resolveJsonModule": true,
    "isolatedModules": true,
    "esModuleInterop": true,
    "noEmit": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "skipLibCheck": true,
    "experimentalDecorators": true,
  },
  "include": ["src"]
}

3) Create an element in main.ts, this example is from capacitor's doc

import { controller, target } from "@github/catalyst"

@controller
class HelloWorldElement extends HTMLElement {
  @target output: HTMLElement

  greet() {
    this.output.textContent = `Hello, world!`
  }
}

4) Run npm run build. Ts fails to compile:

src/main.ts:4:7 - error TS6196: 'HelloWorldElement' is declared but never used.

4 class HelloWorldElement extends HTMLElement {
        ~~~~~~~~~~~~~~~~~

src/main.ts:5:11 - error TS2564: Property 'output' has no initializer and is not definitely assigned in the constructor.

5   @target output: HTMLElement
            ~~~~~~

Found 2 errors in the same file, starting at: src/main.ts:4
jeromebon commented 2 years ago

If I set noUnusedLocals to false then the first error is fixed. If I set strict to false then the second one is solved. Even through it compiles it still doesn't work in the browser. If I take the first example from https://github.github.io/catalyst/guide/actions/ I get this error in the browser: Uncaught TypeError: this.name is undefined

Is there workarounds to use catalyst with theses options set to true ?

I'm not familiar enought with catalyst to be sure, but I feel like the first error could be avoided if there is a way for catalyst to mark a class as used when decorated by @controller and that it's something we would like to do if possible. For the second one i'm unsure, because output is indeed not initialized until connectedCallback is called then typescript error is correct. Should the typing in the doc be updated to @target output: HTMLElement | undefined; ?

keithamus commented 2 years ago

Thanks for the issue @jeromebon!

Let's look at the three errors separately:

'HelloWorldElement' is declared but never used.

This is a bit of a failing with TypeScript, as it doesn't consider that @controller is side-effectful. There's a TS issue around this: https://github.com/microsoft/TypeScript/issues/13120. The solution is to either export the class:

import { controller, target } from "@github/catalyst"

@controller
export class HelloWorldElement extends HTMLElement {

Or to disable the noUnusedLocals option, and instead use a linter that is more robust to decorator usage.

Property 'output' has no initializer and is not definitely assigned in the constructor

We have a section in the guide about the initializer error here. I'll quote it here:

Disabling strictPropertyInitialization

TypeScript comes with various "strict" mode settings, one of which is strictPropertyInitialization which TypeScript catch potential class properties which might not be assigned during construction of a class. This option conflicts with Catalyst's @target/@targets decorators, which safely do the assignment but TypeScript's simple heuristics cannot detect this. It's recommended to disable this option (other strict mode rules can still apply) in your tsconfig.json like so:

{
  "compilerOptions": {
    "strict": true,
    "strictPropertyInitialization": false
  }
}

If you really want to keep the strictPropertyInitialization option set to true, another option would be to use TypeScript's non-null assertion operator (!), adding this to each of your @target/@targets properties, like so:

class HelloWorldElement extends HTMLElement {
  @target something!: HTMLElement
  @targets items!: HTMLElement[]
}

You can also adjust the type to be @target output: HTMLElement | undefined if you want. This is probably more accurate type as we cannot guarantee DOM children exist, but we keep the examples free of |undefined for pragmatism: if you're authoring the HTML and the JS then it's very likely going to be there.

Uncaught TypeError: this.name is undefined

This sounds like it may be an error with Vite or TypeScript minifying the class names. We have a section in the guide about minifier setup here, I'll quote what I think the relevant piece is:

When using ESBuild you can turn off all class and function name minification with the keep_names option. Setting this to true in your build will opt-out all classes and all functions from minification.

{ keep_names: true }
// Or --keep-names on the CLI

Let me know if these solutions work for you, and if not I'd love to find out more. If you think our guide can be improved any more to call these out more prominently, then please let me know, or better yet: PRs welcome!

keithamus commented 1 year ago

I'm going to close this issue, as I believe the above comment should solve the issues mentioned. If you feel otherwise please let me know, and perhaps we can break out each individual area of friction into a separate issue.