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: specify type #1795

Merged
merged 4 commits into from
Jul 19, 2024
Merged

fix: specify type #1795

merged 4 commits into from
Jul 19, 2024

Conversation

kyechan99
Copy link
Contributor

Hello 😄

Is there a reason for not specifying the type for the following changes?
I changed it to use it more accurately.
Thx.

Path

Before

export declare class Path extends Shape<PathConfig> {
    // ...
    static getPointAtLengthOfDataArray(length: number, dataArray: any): {
        x: any;
        y: any;
    } | null;
    static getPointOnLine(dist: any, P1x: any, P1y: any, P2x: any, P2y: any, fromX?: any, fromY?: any): {
        x: any;
        y: any;
    };
}

After

export declare class Path extends Shape<PathConfig> {
    // ...
    static getPointAtLengthOfDataArray(length: number, dataArray: any): Vector2d | null;
    static getPointOnLine(dist: any, P1x: any, P1y: any, P2x: any, P2y: any, fromX?: any, fromY?: any): Vector2d;
}

Text

Before

export declare class Text extends Shape<TextConfig> {
    // ...
    measureSize(text: any): {
        actualBoundingBoxAscent: any;
        actualBoundingBoxDescent: any;
        actualBoundingBoxLeft: any;
        actualBoundingBoxRight: any;
        alphabeticBaseline: any;
        emHeightAscent: any;
        emHeightDescent: any;
        fontBoundingBoxAscent: any;
        fontBoundingBoxDescent: any;
        hangingBaseline: any;
        ideographicBaseline: any;
        width: any;
        height: number;
    };
}

After

export declare class Text extends Shape<TextConfig> {
    // ...
    measureSize(text: string): {
        actualBoundingBoxAscent: number;
        actualBoundingBoxDescent: number;
        actualBoundingBoxLeft: number;
        actualBoundingBoxRight: number;
        alphabeticBaseline: number;
        emHeightAscent: number;
        emHeightDescent: number;
        fontBoundingBoxAscent: number;
        fontBoundingBoxDescent: number;
        hangingBaseline: number;
        ideographicBaseline: number;
        width: number;
        height: number;
    };
}

@lavrton
Copy link
Member

lavrton commented Jul 18, 2024

Thanks for the PR request. The text changes look good.
For path changes, I prefer not to specify type of return value, because it should be automatically detected by typescript. So if it is not correct (like x is any, instead of number) need to look into internals and fix types inside. Can you do that?

@kyechan99
Copy link
Contributor Author

Understand.
Reverted Path changes.

@lavrton
Copy link
Member

lavrton commented Jul 19, 2024

Well, it may be still good to have some changes, like to check why x and y are any while they should be number, do you want to do that?

@kyechan99
Copy link
Contributor Author

OK.
How about this?
It can find out that x and y are numbers without specifying a return type.
Tell me if it's different from what you think.

Exampe

  static getPointOnCubicBezier(pct, P1x, P1y, P2x, P2y, P3x, P3y, P4x, P4y) {
    function CB1(t: number) {   // <- argument type
      return t * t * t;
    }
    function CB2(t: number) {   // <- argument type
      return 3 * t * t * (1 - t);
    }
    function CB3(t: number) {   // <- argument type
      return 3 * t * (1 - t) * (1 - t);
    }
    function CB4(t: number) {   // <- argument type
      return (1 - t) * (1 - t) * (1 - t);
    }
    var x = P4x * CB1(pct) + P3x * CB2(pct) + P2x * CB3(pct) + P1x * CB4(pct);
    var y = P4y * CB1(pct) + P3y * CB2(pct) + P2y * CB3(pct) + P1y * CB4(pct);

    return {
      x: x,  // number
      y: y,  // number
    };
  }

Path.d.ts

Path.getPointOnCubicBezier(pct: any, P1x: any, P1y: any, P2x: any, P2y: any, P3x: any, P3y: any, P4x: any, P4y: any): {
    x: number;
    y: number;
}

To use

Path.getPointOnCubicBezier('0', '0', '0', '0', '0', '0', '0', '0', '0');
Path.getPointOnCubicBezier(0, 0, 0, 0, 0, 0, 0, 0, 0);

@lavrton
Copy link
Member

lavrton commented Jul 19, 2024

Well, as I can see, the type of getPointOnCubicBezier is already { x: number; y: number; }. So we don't need to do much here with the return type. But I don't think the arguments are correct here. Currently, they have no types. So they are any, but I think only numbers should be allowed there.

As I can see getPointOnLine has not 100% corect return, Also getPointAtLength, getPointAtLengthOfDataArray

@kyechan99
Copy link
Contributor Author

Yeah, youre right getPointOnCubicBezier doesn't need any modification.

To modify the three functions (return { x: any; y: any; })
I can only think of two ways to solve it.

  1. Specifying the type of argument
  2. Using Number or parseInt.

If there is a better way, please let me know.

static getPointOnLine(
    dist: number,
    P1x: number,
    P1y: number,
    P2x: number,
    P2y: number,
    fromX?: number,
    fromY?: number
  ) { ... } 

@lavrton
Copy link
Member

lavrton commented Jul 19, 2024

Specifying the type of argument

This is the way.

Also, some functions may need additional types inside their body.

@lavrton lavrton merged commit 9830a6c into konvajs:master Jul 19, 2024
2 checks passed
@lavrton
Copy link
Member

lavrton commented Jul 19, 2024

Thanks

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