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

feat(types): update types for v7 #4186

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

feat(types): update types for v7 #4186

wants to merge 18 commits into from

Conversation

wjhsf
Copy link
Contributor

@wjhsf wjhsf commented Apr 30, 2024

Details

Things that changed

  • lwc/index.js just re-exports from @lwc/engine-dom. lwc/index.d.ts contains numerous outdated types. This removes them. Fixes Have the lwc package directly export types from @lwc/engine-dom #3598.
  • Our decorators currently use the type signature for TypeScript's experimental decorators. But TypeScript introduced new decorators in v5 that are compliant with the proposed spec. This updates the signature of our decorators to support both formats.
    • To validate that the signatures work with both kinds of decorators, check out feat: typescript support! #4178 (or fbc1fe5, if the latest commit is broken). Toggle the experimentalDecorators flag in playground/tsconfig.json and run yarn build for both options.
  • Added a new helper, lwc/html, that provides the correct types for HTML templates. Just import it once in your project (or add it to your tsconfig includes) to get HTML templates to be correctly typed.
  • Narrowed this.template.host to HTMLElement instead of the broader Element, since we know that this will always be true.
  • In v6, WireAdapter and WireAdapterConstructor were generic when imported from lwc, but not when imported from @lwc/engine-core. I updated the type definitions to be generic with defaults, so that both imports should continue to work unmodified.

Things that might break

AKA things that I saw break in our nucleus downstreams.

Breakage Fixage
HTML imports (import myComponent from './my-component.html') Import lwc/html or lwc/@engine-dom/html somewhere in your project.
StringKeyedRecord Replace with Record<string, any> or something better.
ContextValue Renamed to WireContextValue, but it's really just Record<string, any>, so maybe use something better.
ContextConsumer Renamed to WireContextConsumer
Contextualizer Renamed to WireContextProvider
createElement Use type assertion to cast to HTMLElement & MyComponentApiDecoratedProps. You have to define your own interface to get the @api decorated props, because TypeScript can't detect that. (You can use your component class, but that includes all component and LightningElement props, not just component @api decorated props, so it's not ideal.)
this.template is possibly undefined Use optional chaining (?.) or non-null assertion (!.)
Cannot find name ClassMemberDecoratorContext (or other decorator type) Use TypeScript v5.4.5
Other type errors? Use TypeScript v5.4.5

Does this pull request introduce a breaking change?

  • 💔 Yes, it does introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.
  • 🔬 Yes, it does include an observable change.

GUS work item

@wjhsf wjhsf added the nomerge label Apr 30, 2024
@wjhsf wjhsf requested a review from a team as a code owner April 30, 2024 18:54
@nolanlawson
Copy link
Contributor

I'm supportive of this PR in general, so that we can finally resolve #3598. If there are breakages then that's okay, as long as the breakages make sense.

Here is a list of downstream breakages. Let's take some time to make sure these are all reasonable.

@nolanlawson nolanlawson added this to the 7.0.0 milestone Apr 30, 2024
@nolanlawson
Copy link
Contributor

Also: I'm wondering if it's about time we added some real TypeScript tests somewhere. Nothing too fancy, just something to confirm that the types compile the way we expect them to compile. A good place for this would probably the barrel lwc package, since that's where people tend to import from.

@wjhsf
Copy link
Contributor Author

wjhsf commented May 17, 2024

Also: I'm wondering if it's about time we added some real TypeScript tests somewhere. Nothing too fancy, just something to confirm that the types compile the way we expect them to compile. A good place for this would probably the barrel lwc package, since that's where people tend to import from.

Any test written in TypeScript is implicitly a test of the types, as changing the types will cause the tests to fail. So, provided we write enough tests in TypeScript, we get type coverage for free.

@nolanlawson
Copy link
Contributor

@wjhsf The challenge there is that, historically, we don't have any TS-based browser tests in this repo. All our Karma tests are in JS, and our Jest tests (which do use TS) don't use JSDOM or browser APIs.

So for example, we're not testing this kind of code anywhere:

export default class extends LightningElement {
  static renderMode = 'light' // should fail TS
  renderedCallback() {
    this.doesNotExist = foo // should fail TS
    this.setAttribute(123, false) // should fail TS
  }
}

I'm wondering if it would make sense to have a "hello world" Jest+JSDOM test in the lwc package just to test the types. Or tsd or something.

packages/lwc/html.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

This is starting to look really good, but I have some suggestions before merging this:

  1. We need to have at least some minimal tests, preferably in the lwc package, that test the types our users are likely to use.
  2. We should figure out some way to make createElement("x-foo", { is: Foo }).bar work (where bar is some @api-decorated property). There has to be some TypeScript jujitsu that can accomplish this.

@wjhsf
Copy link
Contributor Author

wjhsf commented May 20, 2024

  1. We need to have at least some minimal tests, preferably in the lwc package, that test the types our users are likely to use.

I added it under @lwc/integration-types so that we have more flexibility with the setup (particularly tsconfig settings). 🤷

  1. We should figure out some way to make createElement("x-foo", { is: Foo }).bar work (where bar is some @api-decorated property). There has to be some TypeScript jujitsu that can accomplish this.

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

Successfully merging this pull request may close these issues.

Have the lwc package directly export types from @lwc/engine-dom
2 participants