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

dtslint errors #789

Closed
minggangw opened this issue May 17, 2021 · 2 comments · Fixed by #795
Closed

dtslint errors #789

minggangw opened this issue May 17, 2021 · 2 comments · Fixed by #795
Assignees

Comments

@minggangw
Copy link
Member

Error: /root/rclnodejs/test/types/main.ts:383:1

ERROR: 383:1  expect  TypeScript@4.4 expected type to be:

  number[] | Uint8Array

got:

  Uint8Array | number[]

/root/rclnodejs/test/types/test.ts:9:1

ERROR: 9:1    expect  TypeScript@4.4 expected type to be:

  number[] | Uint8Array

got:

  Uint8Array | number[]

ERROR: 16:1   expect  TypeScript@4.4 expected type to be:

  number[] | Uint8Array

got:

  Uint8Array | number[]

See details: https://travis-ci.org/github/RobotWebTools/rclnodejs/builds/771382735#L27056

@wayneparrott would you please have a look if you have time, thanks!

@wayneparrott wayneparrott self-assigned this May 24, 2021
@wayneparrott
Copy link
Collaborator

wayneparrott commented Jun 2, 2021

This issue is caused by this open dtslint bug issue dating to 2019.

This closed DTSLint issues outlines the strategy for the $ExpectType parser expecting union types to be sorted alphabetically:
microsoft/dtslint#61

As we can see from the error msg $ExpectType is looking for the std_msgs.msg.UInt8MultiArray.data to be of type Uint8Array | number[]. Yet the definition generated by the generate_tsd.js script is:

export interface UInt8MultiArray {
        layout: std_msgs.msg.MultiArrayLayout;
        data: number[] | Uint8Array;
      }

Possible solutions to this issues are:

  1. Simplest solution is to avoid running these 2 tests until the DTSLint relaxes this sorting requirement.
  2. Modify the generate_tsd.js to sort union fields according to Use a consistent ordering when writing union types microsoft/TypeScript#17944

As we are generating a ts declaration file for the ros messages, I believe we should strive to conform to the strategy used by tsc to generate declaration files. I'm looking into option 2 to assess the difficulty and risk. I don't think it will be too much work.

@wayneparrott
Copy link
Collaborator

I propose we disable DTSLint $ExpectType tests for union types until the union type ordering of the $ExpectType parser is better understood.

tldr;
Upon further research and testing I am unable to determine the exact strategy for union type ordering used by DTSLint. I noticed the React team disabled (commented out) their test cases that involved $ExpectType . For example in some cases:

export interface UInt8MultiArray {
        layout: std_msgs.msg.MultiArrayLayout;
        data: number[] | Uint8Array;
      }

results in an error expected Uint8Array | number[]. When we reverse the data property union type definition we get an error complaining the opposite. At this time I have not yet produced an ordering of union types that will consistently pass DTSLint analysis for typescript versions 3.9 - 4.4.

Until we know the exact ordering of union types as applied by the ExpectType parser I propose we follow the path of the React team and disable our DTSLint $ExpectType tests for union types.

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 a pull request may close this issue.

2 participants