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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Begin migration to TypeScript #888

Merged
merged 5 commits into from Mar 16, 2024

Conversation

seanpoulter
Copy link
Contributor

Proposed Changes

  • Add TypeScript to check JavaScript
  • Add nyc to check code coverage (to see what I can change with confidence)
  • Add "build" script that can fail in the pipeline
  • Add JSDoc comments to refine types
  • Minor / low risk code changes

Notes

  • I've pushed my WIP so there isn't a huge PR to review.
  • There are still 16 problems reported by TypeScript.
  • The type definitions could probably be extracted into a better spot. 馃

Next Steps

  • Add tests before changing the untested code
  • Research how to resolve this issue with _handle not being defined. I'm not sure what to replace that with. 馃し
    test/only-driver-tests.js:97:21 - error TS2339: Property '_handle' does not exist on type 'ChildProcess'.
    assert(process1._handle);
    

@seanpoulter seanpoulter changed the title chore: Begin migration to TypeScript style: Begin migration to TypeScript Mar 14, 2024
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM 馃憤

Comment on lines +4 to +7
* @template T
* @param {string} fnName
* @param {T} [opts]
* @returns {T}
Copy link
Member

Choose a reason for hiding this comment

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

Can we update this or remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! What direction would you like to take?

I found this function is the hardest part. Here's how it is used in start and install:

async function install(_opts) {
const opts = checkArgs('Install API', _opts);

Given an optional _opts, when we call checkArgs('...', _opts) we want to return that type (or Partial). I got stuck here with the Partial<T>:

/**
 * @template T
 * @param {string} fnName
 * @param {T} opts
 * @returns {Partial<T>}
 */
const checkArgs = (fnName, opts = {}) => {
//                         ^^^^^^^^^
// Type '{}' is not assignable to type 'T'.
//  'T' could be instantiated with an arbitrary type which could be unrelated to '{}'.ts(2322)

Copy link
Member

Choose a reason for hiding this comment

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

oh .. not sure how to do that with JSDoc, in TS this would look like:

const checkArgs<T extends object> = (fnName: string, opts: T = {}): T => {
  debug(fnName + ' called with', opts);

  return { ...opts };
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I wanted to work in JS so I could work in smaller steps. Would you be OK with the change knowing that it only changes what is logged in the debug call from {} to undefined? I added tests before I gave up on JSDoc. 馃槅

--

I following microsoft/TypeScript#41768 which says that:

<T extends object>

would become

 * @template {object} T

but TS infers it as unknown:

/**
 * @template {object} T
 * @param {string} fnName
 * @param {T} [opts]
 * @returns {T}
 */
const checkArgs = (fnName, opts = {}) => {
//                         ^^^^^^^^^
// Type '{}' is not assignable to type 'T'.
//  '{}' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'unknown'.ts(2322)

It is similar with @template {object} T but you get {} instead of unknown:

Type '{}' is not assignable to type 'T'.
  '{}' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{}'.ts(2322)

That's where I gave up, added a test, and removed = {}. We could also inline the method to remove the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this should not blocking us from moving forward.

@seanpoulter seanpoulter merged commit 052ee21 into webdriverio:main Mar 16, 2024
2 checks passed
@seanpoulter seanpoulter deleted the maintenance branch March 18, 2024 12:58
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

2 participants