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

Layer: TypeScript error with latest @types/react version #4382

Closed
1 task done
Lukas742 opened this issue Apr 3, 2024 · 19 comments
Closed
1 task done

Layer: TypeScript error with latest @types/react version #4382

Lukas742 opened this issue Apr 3, 2024 · 19 comments
Labels
bug General bug label typescript PRs or Issues surrounding Types or TS refactoring
Milestone

Comments

@Lukas742
Copy link

Lukas742 commented Apr 3, 2024

  • I have searched the issues of this repository and believe that this is not a duplicate.

Reproduction link

https://stackblitz.com/edit/vitejs-vite-raiemq?file=package.json,src%2FApp.tsx

Steps to reproduce

When building recharts without skipLibCheck: true a TypeScript error is thrown with v18.2.74 of @types/react.
I believe the root cause is how TypeScript infers and represents types for forwardRef, but unfortunately I wasn't able to fix it myself.

  1. Go to StackBlitz
  2. run tsc in terminal
  3. See the following error:
node_modules/recharts/types/container/Layer.d.ts:7:108 - error TS2344: Type '"string" | "filter" | "values" | "fill" | "stroke" | "max" | "type" | "accumulate" | "offset" | "key" | "id" | "media" | "origin" | "height" | "width" | "end" | "name" | "min" | "alignmentBaseline" | ... 450 more ... | "onPointerLeaveCapture"' does not satisfy the constraint '"string" | "filter" | "values" | "fill" | "stroke" | "max" | "type" | "accumulate" | "offset" | "key" | "id" | "media" | "origin" | "height" | "width" | "end" | "name" | "min" | "alignmentBaseline" | ... 457 more ... | "onTransitionEndCapture"'.
  Type '"onPointerEnterCapture"' is not assignable to type '"string" | "filter" | "values" | "fill" | "stroke" | "max" | "type" | "accumulate" | "offset" | "key" | "id" | "media" | "origin" | "height" | "width" | "end" | "name" | "min" | "alignmentBaseline" | ... 457 more ... | "onTransitionEndCapture"'. Did you mean '"onPointerOverCapture"'?

7 export declare const Layer: React.ForwardRefExoticComponent<Pick<React.SVGProps<SVGGElement> & LayerProps, "string" | "ideographic" | "alphabetic" | "hanging" ...[shortened for readability]> & React.RefAttributes<unknown>>;

What is expected?

Types for React v18 should be supported.

What is actually happening?

Error is thrown.

Environment Info
Recharts v2.12.3
React ^18.2.0
System macOS 14.3.1 (23D60)
Browser -

"@types/react": "18.2.74"

@ckifer ckifer added bug General bug label typescript PRs or Issues surrounding Types or TS refactoring labels Apr 3, 2024
@ckifer
Copy link
Member

ckifer commented Apr 3, 2024

Thanks for the sandbox, I can reproduce.

Generally skipLibCheck is recommended so that things like this do not break you - otherwise I think everything works fine, this type and file has not changed in a long time.

Either way, we'll use this to track the issue and hopefully be able to resolve it

@ckifer
Copy link
Member

ckifer commented Apr 3, 2024

Looks like the xyzCapture events were removed in react types in 18.x because they don't do anything.

Relevant react issue
facebook/react#17883

Our generated declaration files include these removed events which breaks you with skipLibCheck: true.

We'll upgrade react types in 3.x and this shouldn't be an issue anymore, but probably won't do it in 2.x for risk of breaking something else.

Thanks!

CC: @PavelVanecek

@ckifer ckifer added this to the Recharts@3.0 milestone Apr 3, 2024
@Lukas742
Copy link
Author

Lukas742 commented Apr 4, 2024

Thanks @ckifer!

I did a bit more digging and came to the following result, maybe it helps:

Typically, type discrepancies shouldn't be an issue, as long as only the imported types are used, allowing consumers to define the version of, e.g. @types/react on their end. In this case, it appears that the forwardRef types are transformed to leverage the SVGProps' keys for the Pick utility type. Consequently, these keys are bundled in with the @types/react version used in this project. If a type from SVGProps (or any interface it extends) is removed, the described error is thrown.

@dkilgore-eightfold
Copy link

I get the same linter error for placeholder and onPointerLeaveCapture (in addition to the onPointerEnterCapture error) on a component deriving props from HTMLDivElement. Good to know this is a typing error and nothing to do with my component. A fix would be much appreciated.

@HHogg
Copy link

HHogg commented Apr 10, 2024

👋 @ckifer I see there is a project for v3, but is there any information anywhere on the timeline for when it's going to be released and if it's going to be including many other breaking changes?

Just wondering if it could be worth doing a small fix for this on the v2 release to help folks resolve this without needing to do any migration to v3?

@PavelVanecek
Copy link
Collaborator

@HHogg it will still be couple of months until 3.x is ready. Yeah we can do 2.x release as well.

What would the fix look like?

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

What Pavel said - I'm not sure if there is any fix except upgrading React types? I'm also unsure if doing so breaks anyone on 16.8 or 17

@HHogg
Copy link

HHogg commented Apr 10, 2024

👋 Similar to what Lukas742 above said. Taking a look at what we have in node_modules, we see that the keys are all being embedded and used in the Pick...

// types/container/Layer.d.ts
import React, { ReactNode, SVGProps } from 'react';
interface LayerProps {
    className?: string;
    children?: ReactNode;
}
export declare type Props = SVGProps<SVGGElement> & LayerProps;
export declare const Layer: React.ForwardRefExoticComponent<Pick<React.SVGProps<SVGGElement> & LayerProps, "string" | "children" | "dangerouslySetInnerHTML" | "onCopy" | "onCopyCapture" | "onCut" | "onCutCapture" | "onPaste" | "onPasteCapture" | "onCompositionEnd" | "onCompositionEndCapture" | "onCompositionStart" | "onCompositionStartCapture" | "onCompositionUpdate" | "onCompositionUpdateCapture" | "onFocus" | "onFocusCapture" | "onBlur" | "onBlurCapture" | "onChange" | "onChangeCapture" | "onBeforeInput" | "onBeforeInputCapture" | "onInput" | "onInputCapture" | "onReset" | "onResetCapture" | "onSubmit" | "onSubmitCapture" | "onInvalid" | "onInvalidCapture" | "onLoad" | "onLoadCapture" | "onError" | "onErrorCapture" | "onKeyDown" | "onKeyDownCapture" | "onKeyPress" | "onKeyPressCapture" | "onKeyUp" | "onKeyUpCapture" | "onAbort" | "onAbortCapture" | "onCanPlay" | "onCanPlayCapture" | "onCanPlayThrough" | "onCanPlayThroughCapture" | "onDurationChange" | "onDurationChangeCapture" | "onEmptied" | "onEmptiedCapture" | "onEncrypted" | "onEncryptedCapture" | "onEnded" | "onEndedCapture" | "onLoadedData" | "onLoadedDataCapture" | "onLoadedMetadata" | "onLoadedMetadataCapture" | "onLoadStart" | "onLoadStartCapture" | "onPause" | "onPauseCapture" | "onPlay" | "onPlayCapture" | "onPlaying" | "onPlayingCapture" | "onProgress" | "onProgressCapture" | "onRateChange" | "onRateChangeCapture" | "onSeeked" | "onSeekedCapture" | "onSeeking" | "onSeekingCapture" | "onStalled" | "onStalledCapture" | "onSuspend" | "onSuspendCapture" | "onTimeUpdate" | "onTimeUpdateCapture" | "onVolumeChange" | "onVolumeChangeCapture" | "onWaiting" | "onWaitingCapture" | "onAuxClick" | "onAuxClickCapture" | "onClick" | "onClickCapture" | "onContextMenu" | "onContextMenuCapture" | "onDoubleClick" | "onDoubleClickCapture" | "onDrag" | "onDragCapture" | "onDragEnd" | "onDragEndCapture" | "onDragEnter" | "onDragEnterCapture" | "onDragExit" | "onDragExitCapture" | "onDragLeave" | "onDragLeaveCapture" | "onDragOver" | "onDragOverCapture" | "onDragStart" | "onDragStartCapture" | "onDrop" | "onDropCapture" | "onMouseDown" | "onMouseDownCapture" | "onMouseEnter" | "onMouseLeave" | "onMouseMove" | "onMouseMoveCapture" | "onMouseOut" | "onMouseOutCapture" | "onMouseOver" | "onMouseOverCapture" | "onMouseUp" | "onMouseUpCapture" | "onSelect" | "onSelectCapture" | "onTouchCancel" | "onTouchCancelCapture" | "onTouchEnd" | "onTouchEndCapture" | "onTouchMove" | "onTouchMoveCapture" | "onTouchStart" | "onTouchStartCapture" | "onPointerDown" | "onPointerDownCapture" | "onPointerMove" | "onPointerMoveCapture" | "onPointerUp" | "onPointerUpCapture" | "onPointerCancel" | "onPointerCancelCapture" | "onPointerEnter" | "onPointerEnterCapture" | "onPointerLeave" | "onPointerLeaveCapture" | "onPointerOver" | "onPointerOverCapture" | "onPointerOut" | "onPointerOutCapture" | "onGotPointerCapture" | "onGotPointerCaptureCapture" | "onLostPointerCapture" | "onLostPointerCaptureCapture" | "onScroll" | "onScrollCapture" | "onWheel" | "onWheelCapture" | "onAnimationStart" | "onAnimationStartCapture" | "onAnimationEnd" | "onAnimationEndCapture" | "onAnimationIteration" | "onAnimationIterationCapture" | "onTransitionEnd" | "onTransitionEndCapture" | "className" | "color" | "height" | "id" | "lang" | "max" | "media" | "method" | "min" | "name" | "style" | "target" | "type" | "width" | "role" | "tabIndex" | "crossOrigin" | "accentHeight" | "accumulate" | "additive" | "alignmentBaseline" | "allowReorder" | "alphabetic" | "amplitude" | "arabicForm" | "ascent" | "attributeName" | "attributeType" | "autoReverse" | "azimuth" | "baseFrequency" | "baselineShift" | "baseProfile" | "bbox" | "begin" | "bias" | "by" | "calcMode" | "capHeight" | "clip" | "clipPath" | "clipPathUnits" | "clipRule" | "colorInterpolation" | "colorInterpolationFilters" | "colorProfile" | "colorRendering" | "contentScriptType" | "contentStyleType" | "cursor" | "cx" | "cy" | "d" | "decelerate" | "descent" | "diffuseConstant" | "direction" | "display" | "divisor" | "dominantBaseline" | "dur" | "dx" | "dy" | "edgeMode" | "elevation" | "enableBackground" | "end" | "exponent" | "externalResourcesRequired" | "fill" | "fillOpacity" | "fillRule" | "filter" | "filterRes" | "filterUnits" | "floodColor" | "floodOpacity" | "focusable" | "fontFamily" | "fontSize" | "fontSizeAdjust" | "fontStretch" | "fontStyle" | "fontVariant" | "fontWeight" | "format" | "fr" | "from" | "fx" | "fy" | "g1" | "g2" | "glyphName" | "glyphOrientationHorizontal" | "glyphOrientationVertical" | "glyphRef" | "gradientTransform" | "gradientUnits" | "hanging" | "horizAdvX" | "horizOriginX" | "href" | "ideographic" | "imageRendering" | "in2" | "in" | "intercept" | "k1" | "k2" | "k3" | "k4" | "k" | "kernelMatrix" | "kernelUnitLength" | "kerning" | "keyPoints" | "keySplines" | "keyTimes" | "lengthAdjust" | "letterSpacing" | "lightingColor" | "limitingConeAngle" | "local" | "markerEnd" | "markerHeight" | "markerMid" | "markerStart" | "markerUnits" | "markerWidth" | "mask" | "maskContentUnits" | "maskUnits" | "mathematical" | "mode" | "numOctaves" | "offset" | "opacity" | "operator" | "order" | "orient" | "orientation" | "origin" | "overflow" | "overlinePosition" | "overlineThickness" | "paintOrder" | "panose1" | "path" | "pathLength" | "patternContentUnits" | "patternTransform" | "patternUnits" | "pointerEvents" | "points" | "pointsAtX" | "pointsAtY" | "pointsAtZ" | "preserveAlpha" | "preserveAspectRatio" | "primitiveUnits" | "r" | "radius" | "refX" | "refY" | "renderingIntent" | "repeatCount" | "repeatDur" | "requiredExtensions" | "requiredFeatures" | "restart" | "result" | "rotate" | "rx" | "ry" | "scale" | "seed" | "shapeRendering" | "slope" | "spacing" | "specularConstant" | "specularExponent" | "speed" | "spreadMethod" | "startOffset" | "stdDeviation" | "stemh" | "stemv" | "stitchTiles" | "stopColor" | "stopOpacity" | "strikethroughPosition" | "strikethroughThickness" | "stroke" | "strokeDasharray" | "strokeDashoffset" | "strokeLinecap" | "strokeLinejoin" | "strokeMiterlimit" | "strokeOpacity" | "strokeWidth" | "surfaceScale" | "systemLanguage" | "tableValues" | "targetX" | "targetY" | "textAnchor" | "textDecoration" | "textLength" | "textRendering" | "to" | "transform" | "u1" | "u2" | "underlinePosition" | "underlineThickness" | "unicode" | "unicodeBidi" | "unicodeRange" | "unitsPerEm" | "vAlphabetic" | "values" | "vectorEffect" | "version" | "vertAdvY" | "vertOriginX" | "vertOriginY" | "vHanging" | "vIdeographic" | "viewBox" | "viewTarget" | "visibility" | "vMathematical" | "widths" | "wordSpacing" | "writingMode" | "x1" | "x2" | "x" | "xChannelSelector" | "xHeight" | "xlinkActuate" | "xlinkArcrole" | "xlinkHref" | "xlinkRole" | "xlinkShow" | "xlinkTitle" | "xlinkType" | "xmlBase" | "xmlLang" | "xmlns" | "xmlnsXlink" | "xmlSpace" | "y1" | "y2" | "y" | "yChannelSelector" | "z" | "zoomAndPan" | "aria-activedescendant" | "aria-atomic" | "aria-autocomplete" | "aria-busy" | "aria-checked" | "aria-colcount" | "aria-colindex" | "aria-colspan" | "aria-controls" | "aria-current" | "aria-describedby" | "aria-details" | "aria-disabled" | "aria-dropeffect" | "aria-errormessage" | "aria-expanded" | "aria-flowto" | "aria-grabbed" | "aria-haspopup" | "aria-hidden" | "aria-invalid" | "aria-keyshortcuts" | "aria-label" | "aria-labelledby" | "aria-level" | "aria-live" | "aria-modal" | "aria-multiline" | "aria-multiselectable" | "aria-orientation" | "aria-owns" | "aria-placeholder" | "aria-posinset" | "aria-pressed" | "aria-readonly" | "aria-relevant" | "aria-required" | "aria-roledescription" | "aria-rowcount" | "aria-rowindex" | "aria-rowspan" | "aria-selected" | "aria-setsize" | "aria-sort" | "aria-valuemax" | "aria-valuemin" | "aria-valuenow" | "aria-valuetext" | "key"> & React.RefAttributes<unknown>>;
export {};

I think it's due to using SVGProps rather than SVGAttributes. I stumbled upon AntD, making a similar change. Which after making and running another build, I get something more like I would expect to see.

// types/container/Layer.d.ts
import React, { ReactNode, SVGAttributes } from 'react';
interface LayerProps {
  className?: string;
  children?: ReactNode;
}
export type Props = SVGAttributes<SVGGElement> & LayerProps;
export declare const Layer: React.ForwardRefExoticComponent<
  React.SVGAttributes<SVGGElement> & LayerProps & React.RefAttributes<unknown>
>;
export {};

@HHogg
Copy link

HHogg commented Apr 10, 2024

Also worth noting, it's only Layer that has this problem, looking at all the other generated type definitions they seem to be fine.

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

Pretty much all components extend SVG props one way or another so I'm not sure why that's the case. If we change it one place we should change it everywhere

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

I'll take a look at things at some point today but I want some sort of guarantee that only this component is affected and a simple change such as the one proposed will fix it so we can avoid multiple patches (and avoid this in the future)

Type changes have a way of making things break. Just want to sanity check that we won't break more than this already does with a change

@HHogg
Copy link

HHogg commented Apr 10, 2024

Yeah that makes sense @ckifer, thanks for taking a look at this. I've had a quick look and here's my thinking. The SVGProps interface...

interface SVGProps<T> extends SVGAttributes<T>, ClassAttributes<T>

... is SVGAttributes anyway with ClassAttributes, which is

interface ClassAttributes<T> extends Attributes {
  ref?: LegacyRef<T> | undefined;
}

interface Attributes {
  key?: Key | null | undefined;
}

... so the ref and key which aren't part of SVGAttributes need to be added, and are added implicitly from createElement. and you can see that Attributes and ClassAttributes are added anyway, which is why key and ref are always available in your IDE even when you don't specify it.

const MyComponent = () => null;
const Foo = () => <MyComponent key="IT_WORKS" />

const MyComponentForwarded = forwardRef(() => null);
const Bar = () => <MyComponentForwarded key="IT_WORKS" ref={useRef()} />;

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

Thanks @HHogg that helps.

I'm still confused why this is only an issue for Layer

Most components props extend this which contains SVGProps. Most other components have onPointerEnterCapture, etc. as acceptable props because of that. Why do things not fail there as well? (saying this out loud before investigating, I'll start investigating when I get time)

@HHogg
Copy link

HHogg commented Apr 10, 2024

That I'm not really sure about, other than assuming there's some tooling that is being "helpful" with optimizing it at build time for some reason.

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

Ok so double checking everything everything:

  • This only happens for components that use forwardRef (noted above)
  • we only do this twice
    • Layer -> this issue
    • ResponsiveContainer -> The same problem does not seem to happen with HTMLDivElement, only SVGProps causes the issue. If I do something silly like make ResponsiveContainerProps extend SVG props, we have the same result. It is unclear why, but 🤷🏼‍♂️
  • Removing SVGProps isn't an issue due to what was mentioned above - ref and key are added implicitly in React (forwardRef also adds ref for us)
  • Autocomplete still works for all applicable SVG types (still need to update @types/react in 3.x of course)
  • No urgent need to replace the other usage of SVGProps in 2.x, but we should use SVGAttributes over SVGProps moving forwards

Confirmed the SVGAttributes fix works with a local publish using yalc

@ckifer
Copy link
Member

ckifer commented Apr 10, 2024

Lmk if this looks okay #4413
(fixing some related types while I'm at it, not sure why Pie was an HTMLElement...)

@HHogg
Copy link

HHogg commented Apr 12, 2024

Hey @ckifer 👋 Would it be ok to pop out a release for this change?

@ckifer
Copy link
Member

ckifer commented Apr 12, 2024

I can yeah, just need to find some free time

@ckifer
Copy link
Member

ckifer commented Apr 12, 2024

Fixed in https://github.com/recharts/recharts/releases/tag/v2.12.5

@ckifer ckifer closed this as completed Apr 12, 2024
renovate bot added a commit to SAP/ui5-webcomponents-react that referenced this issue Apr 15, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [recharts](https://togithub.com/recharts/recharts) | [`2.12.4` ->
`2.12.5`](https://renovatebot.com/diffs/npm/recharts/2.12.4/2.12.5) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/recharts/2.12.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/recharts/2.12.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/recharts/2.12.4/2.12.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/recharts/2.12.4/2.12.5?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>recharts/recharts (recharts)</summary>

###
[`v2.12.5`](https://togithub.com/recharts/recharts/releases/tag/v2.12.5)

[Compare
Source](https://togithub.com/recharts/recharts/compare/v2.12.4...v2.12.5)

Small fixes while working on v3 continued...

#### What's Changed

##### Feat

- `BarChart`: support percentage (of chart) for `barSize`. Helps set
size of bar when there are few datapoints Fixes
[#&#8203;3640](https://togithub.com/recharts/recharts/issues/3640) by
[@&#8203;graup](https://togithub.com/graup) in
[recharts/recharts#4407

##### Fix

Address
[recharts/recharts#4382

A recent release of `@types/react` broke some builds because they
removed certain (unused) events from common event handler attributes.
`recharts` was unknowingly enumerating keys of `SVGProps` in the `Layer`
component with the old types and causing a type error on `tsc` with
`skipLibCheck: false`

- `typescript - Layer`: use `SVGAttributes` instead of `SVGProps` in
forwardRef components by [@&#8203;ckifer](https://togithub.com/ckifer)
in
[recharts/recharts#4413
- `typescript - Pie`: fix Pie `ref` which was cast to `HTMLElement` when
the `ref` is actually referring to `SVGGElement`. This gave false
information to whoever is using `ref` on the `Pie` component

**Full Changelog**:
recharts/recharts@v2.12.4...v2.12.5

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/SAP/ui5-webcomponents-react).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug General bug label typescript PRs or Issues surrounding Types or TS refactoring
Projects
None yet
Development

No branches or pull requests

5 participants