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(declarations): use correct TypeScript JSX typings #5307

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maxpatiiuk
Copy link

What is the current behavior?

Fix incorrect JSX typings, as described in great detail in #5306

GitHub Issue Number: #5306

What is the new behavior?

TypeScript compiler is able to resolve Stencil's JSX typings correctly, thus working well with third-party tools that use TypeScript compiler types

Documentation

Described in great detail in #5306

Does this introduce a breaking change?

  • Yes
  • No

Testing

The error affects tools that use the TypeScript compiler. It is unfortunate that typescript does not report this error directly - instead, it just settles for an implicit any, which is how I discovered this bug in the first place.

Thus suggested testing:

  1. In an app that is using Stencil and TypeScript, install typescript-eslint and enable the @typescript-eslint/no-unsafe-return rule
  2. Create a small test file like this:
import { type VNode, h } from "@stencil/core/internal";

export function a(): VNode {
  return <br />;
}

Before this PR there would be an error on line return <br />;. After this PR, there should not be an error there

@maxpatiiuk maxpatiiuk requested a review from a team as a code owner January 31, 2024 04:13
Copy link
Contributor

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1207 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
Path Error Count
src/dev-server/index.ts 37
src/mock-doc/serialize-node.ts 36
src/dev-server/server-process.ts 32
src/compiler/prerender/prerender-main.ts 22
src/testing/puppeteer/puppeteer-element.ts 22
src/runtime/client-hydrate.ts 20
src/screenshot/connector-base.ts 19
src/runtime/vdom/vdom-render.ts 17
src/compiler/config/test/validate-paths.spec.ts 16
src/dev-server/request-handler.ts 15
src/compiler/prerender/prerender-optimize.ts 14
src/compiler/sys/stencil-sys.ts 14
src/compiler/transpile/transpile-module.ts 14
src/sys/node/node-sys.ts 14
src/compiler/prerender/prerender-queue.ts 13
src/compiler/sys/in-memory-fs.ts 13
src/runtime/connected-callback.ts 13
src/runtime/set-value.ts 13
src/compiler/output-targets/output-www.ts 12
src/compiler/transformers/test/parse-vdom.spec.ts 12
Our most common errors
Typescript Error Code Count
TS2345 366
TS2322 362
TS18048 224
TS18047 99
TS2722 37
TS2532 26
TS2531 23
TS2454 14
TS2790 11
TS2352 10
TS2769 8
TS2538 8
TS2344 6
TS2416 6
TS2493 3
TS18046 2
TS2684 1
TS2430 1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports
File Line Identifier
src/runtime/bootstrap-lazy.ts 21 setNonce
src/screenshot/screenshot-fs.ts 18 readScreenshotData
src/testing/testing-utils.ts 198 withSilentWarn
src/utils/index.ts 145 CUSTOM
src/utils/index.ts 269 normalize
src/utils/index.ts 7 escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts 25 BUILD
src/compiler/app-core/app-data.ts 115 Env
src/compiler/app-core/app-data.ts 117 NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts 123 updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts 61 satisfies
src/compiler/types/validate-primary-package-output-target.ts 61 Record
src/testing/puppeteer/puppeteer-declarations.ts 485 WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts 7 writeFetchSuccessSync

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Hey @maxpatiiuk 👋

I created a dev build based on this branch to test it out (you can grab it yourself using npm i @stencil/[email protected]).

Using this dev build, I tested the PR by using it to build the Ionic Framework. There are a few cases where I think this would improve type checking, I noticed a few cases where setting type Element = VNode caused a few breaking changes with the typings used in the Framework today. I'm concerned this might be confusing to developers here if they were to upgrade Stencil and the type checker began to complain.

With that in mind and the current state of this PR, I'd like to revisit this for Stencil v5. I think this is the best case scenario for improving the typings of Stencil, while trying to minimize breaking changes in minor releases.

Now that I understand the issue here/how to reproduce it, I'm going to combine the two issues linked here. In the interim, I suggest adding an any return type to your render functions, as that appears to appease the ESLint rule no-unsafe-return and aligns with the currently typed return value of render

Thanks for your patience and let me know if you have any questions!

@@ -596,12 +596,12 @@ export interface ChildNode {
*
* For further information: https://stenciljs.com/docs/host-element
*/
export declare const Host: FunctionalComponent<HostAttributes>;
Copy link
Member

Choose a reason for hiding this comment

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

While I understand the motivation for this change, I don't think we can make this change in Stencil v4. This changes the output of typeof Host, which would constitute a breaking change.

In Stencil v4, the following code type checks without error:

import { Host } from '@stencil/core';

const Hello: typeof Host = (_props: any, children: any, utils: any) =>
  utils.map(children, child => ({
    ...child,
    vattrs: {
      ...child.vattrs,
      class: `${child.vattrs.class} add-class`,
    },
  }));

Whereas with this commit/change, it fails with:

[ ERROR ]  TypeScript: src/components/my-component/my-component.tsx:4:7
           Type '(_props: any, children: any, utils: any) => any' is not assignable to type '(props: HostAttributes) =>
           VNode'.Target signature provides too few arguments. Expected 3 or more, but got 1.

      L4:  const Hello: typeof Host = (_props: any, children: any, utils: any) =>
      L5:    utils.map(children, child => ({


/**
* Fragment
*/
export declare const Fragment: FunctionalComponent<{}>;
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment above - while I understand the motivation for this change, it would also be a breaking change.

In Stencil v4, the following code type checks without error:

import { Fragment } from '@stencil/core';

const Hello: typeof Fragment = (_props: any, children: any, utils: any) =>
  utils.map(children, child => ({
    ...child,
    vattrs: {
      ...child.vattrs,
      class: `${child.vattrs.class} add-class`,
    },
  }));

Whereas with this commit/change, it fails with:

[ ERROR ]  TypeScript: src/components/my-component/my-component.tsx:4:7
           Type '(_props: any, children: any, utils: any) => any' is not assignable to type '(props: {}) =>
           VNode'.Target signature provides too few arguments. Expected 3 or more, but got 1.

      L4:  const Hello: typeof Fragment = (_props: any, children: any, utils: any) =>
      L5:    utils.map(children, child => ({

@rwaskiewicz rwaskiewicz removed their assignment Feb 2, 2024
@rwaskiewicz rwaskiewicz added the Stencil v5 This is slated for Stencil v5 (Release date TBD) label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stencil v5 This is slated for Stencil v5 (Release date TBD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants