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(types): expose missing JSX.Element on h #5944

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

Andarist
Copy link
Contributor

What is the current behavior?

An accidental any can leak as that's what JSX returns when JSX.Element is missing (TS playground)

// @jsx: react
// @jsxFactory: h
import { h } from '@stencil/core' // types: 4.20.0

const el = <div/> // any

GitHub Issue Number: N/A

What is the new behavior?

This PR reuses the already defined LocalJSX.Element as JSX.Element. The resulting type is still pretty permissive (it's an empty interface) but at least it's not any.

Documentation

N/A

Does this introduce a breaking change?

  • Yes
  • No

Testing

Manual testing of how types are resolved with this.

Other information

discovered here microsoft/TypeScript#59584 (comment)

Copy link
Member

@christian-bromann christian-bromann 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 contribution ❤️

@christian-bromann christian-bromann added this pull request to the merge queue Aug 19, 2024
Merged via the queue into ionic-team:main with commit 958c826 Aug 19, 2024
88 checks passed
christian-bromann added a commit that referenced this pull request Aug 22, 2024
@christian-bromann
Copy link
Member

@Andarist unfortunately I had to revert this change as it causes issues in our Nightly build that runs against the Ionic Framework. Can you please raise an issue for this with a minimal reproducible example?

@Andarist
Copy link
Contributor Author

I see what's going on. So you very much rely on the fact that this returns any.

JSX in TS is incapable (today) of typing the resulting element based on the arguments supplied to JSX. You can only return a static type. You have code like:

private getSVGPath(mode: Mode, indeterminate: boolean): HTMLElement {
  // simplified function body here
  let path = <path d="M6 12L18 12" part="mark" /> 
  return path;
}

So the problem is that with the latest version of Stencil this path is an implicit any. So naturally it's assignable to HTMLElement. The funny thing is that this particular return type annotation is not correct! It actually should be typed as SVGElement.

Based on this, I think that Stencil returns native html/svg elements from its JSX factory (unlike most other frameworks). So naturally you'd like to type those as some kind of concrete element types. This is not possible in TS. You either have to type this as any (and we know how risky this is) or you need to accept this PR (or some variation of it) and switch all call sites that relied on this any to casts.

Side note: I don't quite understand why this h.JSX is different from LocalJSX (which is exported as JSX). Some clarification on this would help me understand better what you are trying to achieve here.

@christian-bromann
Copy link
Member

Thanks for the clarification @Andarist. I would be happy to merge this PR if it doesn't break Ionic Framework as our upstream project. Do you think this is possible?

@Andarist
Copy link
Contributor Author

@christian-bromann we could try with:

export namespace JSX {
    type Element = any;
}

This would, at least, match the current behavior. It would be different from LocalJSX.Element and I don't like that but I don't understand how you are using this LocalJSX (exported as JSX) so it's hard for me to advise on that.

@christian-bromann
Copy link
Member

@Andarist let's try, mind raising a PR for this?

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

Successfully merging this pull request may close these issues.

2 participants