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

fix(create-vite): update template-lit-ts tsconfig (fix #12854) #12855

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

augustjk
Copy link
Contributor

Description

Fixes #12854
Updates tsconfig settings to be compatible with Lit projects.
Context for settings: https://lit.dev/docs/components/decorators/#decorators-typescript

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it. (Not applicable)

Review PR in StackBlitz Codeflow Submitted with StackBlitz Codeflow.

@stackblitz
Copy link

stackblitz bot commented Apr 14, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ArnaudBarre
Copy link
Member

Is this still valid with TS 5?
This flag is now kind of deprecated: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#differences-with-experimental-legacy-decorators

Same for useDefineForClassFields, this flag enforce using a more spec compliant compilation, which I think is good. Unless crucial feature of the framework doesn't work without it, I would be against having empty templates that put you in a situation where code doesn't follow the spec.

@redfox-mx
Copy link

redfox-mx commented Apr 15, 2023

Hi, I think this options must be keep since esbuild is not a full spec typescript compiler. By other hand, Lit docs really recommend this options to be setted.

Why? Currently, lit uses old decorators style since it uses typescript 4.7. useDefineForClassFields is more importan than decorators because lit uses property accesors to keep track of reactivity system, and old options brake it.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I'm fine with this if it's what the lit docs recommend.

@ArnaudBarre
Copy link
Member

I personally feels bad to ship empty templates with legacy/deprecated options turn on. If decorators are not a required part of the framework, I would advice people to not start using this legacy runtime behaviour that will never be shipped to engines. Is this legacy requirement tracked as an issue on the framework side?

@augustjk
Copy link
Contributor Author

Lit does plan to implement the new decorators eventually but we require decorator metadata to be implemented microsoft/TypeScript#53461 for feature parity with our existing decorators.

While technically decorators are not a required part of the framework, most Lit projects using TS will be using decorators as they are prevalent in the official docs and examples as well.

I do understand the hesitation of including legacy/deprecated settings for templates. However, per the TS 5.0 blog

--experimentalDecorators will continue to exist for the foreseeable future

I believe it's valid to include this flag for projects that have relied on this feature for so long. And these flag requirements are documented in the Lit docs as mentioned above.

As it is, https://vite.new/lit-ts leads to a non-functioning project.

Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

Thanks for the update and the pointer.

For more context on my initial pushback, I think we have a great chance in the ecosystem to have access to fast transpiler like esbuild and I think we should try to stay away from TS features that are more than syntax transformation to be able to use these new transpilers.

@patak-dev patak-dev merged commit c186815 into vitejs:main Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[template-lit-ts] Project fails to render
5 participants