RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/jazzy/Concepts/Basic/About-Client-Libraries.html#community-maintained
Apache License 2.0
335 stars 72 forks source link

dtslint errors #789

Closed minggangw closed 3 years ago

minggangw commented 3 years ago
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 commented 3 years ago

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: https://github.com/microsoft/dtslint/pull/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 https://github.com/microsoft/TypeScript/issues/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 commented 3 years ago

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.