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: fix using React.ComponentProps with VictoryLine component #2547

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Waltari10
Copy link

Looking at file myApp/node_modules/victory-line/lib/victory-line.d.ts file the built type file ended up with a constructor that takes props as type of any:

declare class VictoryLineBase extends React.Component<VictoryLineProps> {
    constructor(props: any); // Breaks typing
    static animationWhitelist: string[];

When trying to parse the components props one ended up with any type for VictoryLineProps. However inferring types for other components like VictoryScatter worked fine because it didn't have constructor defined in code.

import {
  VictoryLine,
  VictoryScatter,
} from "victory-native";

type VictoryScatterProps = React.ComponentProps<typeof VictoryScatter>; // Correct typing
type VictoryLineProps = React.ComponentProps<typeof VictoryLine>; // any type

So in this PR just removing the constructor to avoid it getting any type. Doesn't look like the constructor is needed or used for anything anyway.

@changeset-bot
Copy link

changeset-bot bot commented Jan 2, 2023

🦋 Changeset detected

Latest commit: d9e321d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
victory-line Minor
victory-area Minor
victory-axis Minor
victory-bar Minor
victory-box-plot Minor
victory-brush-container Minor
victory-brush-line Minor
victory-candlestick Minor
victory-canvas Minor
victory-chart Minor
victory-core Minor
victory-create-container Minor
victory-cursor-container Minor
victory-errorbar Minor
victory-group Minor
victory-histogram Minor
victory-legend Minor
victory-native Minor
victory-pie Minor
victory-polar-axis Minor
victory-scatter Minor
victory-selection-container Minor
victory-shared-events Minor
victory-stack Minor
victory-tooltip Minor
victory-vendor Minor
victory-voronoi-container Minor
victory-voronoi Minor
victory-zoom-container Minor
victory Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Waltari10 Waltari10 changed the title fix: fix victory line type inferring by removing constructor fix: fix using React.ComponentProps for VictoryLine component Jan 2, 2023
@Waltari10 Waltari10 changed the title fix: fix using React.ComponentProps for VictoryLine component fix: fix using React.ComponentProps with VictoryLine component Jan 2, 2023
@Waltari10
Copy link
Author

Hi, is it possible to get some eyeballs on this PR?.. Would be great to get rid of a patch-package in our codebase 😀

@ryan-roemer @becca-bailey ?

@ryan-roemer
Copy link
Member

@Waltari10 Thanks for this! We'll try and get around to this next week if we can. I'm going to add a review to get CI running and checking.

Copy link
Member

@ryan-roemer ryan-roemer left a comment

Choose a reason for hiding this comment

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

.

ryan-roemer
ryan-roemer previously approved these changes Feb 2, 2023
Copy link
Member

@ryan-roemer ryan-roemer left a comment

Choose a reason for hiding this comment

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

CI

@ryan-roemer ryan-roemer dismissed their stale review February 2, 2023 06:45

Just fiddling with CI

@ryan-roemer
Copy link
Member

@scottrippey @gksander Any idea why this CI is stuck in Expected — Waiting for status to be reported? This is a forked PR, but I don't think that should result in this particular situation... ?

@gksander
Copy link
Contributor

gksander commented Feb 2, 2023

I'm going to try closing and reopening this PR to see if it'll trigger GitHub Actions to send another approval request so we can get CI ran on this!

@gksander gksander closed this Feb 2, 2023
@gksander gksander reopened this Feb 2, 2023
@gksander
Copy link
Contributor

gksander commented Feb 2, 2023

We're going to have to look into why forked PRs aren't triggering approval requests for CI runs, but in the meantime – I went ahead and just pushed up a couple dummy commits to the branch to trigger the workflows.

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.

None yet

3 participants