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

Type fixes & acceptance tests #23

Merged
merged 7 commits into from
Dec 21, 2018
Merged

Conversation

mike-north
Copy link
Contributor

  • Fixes type compile errors introduced by Allow generic types for IConnectionObject #22
  • Type-safety around "promisified" methods object, invoked through postMessage channel (see included tests that demonstrate this)
  • Type information is now tested via dtslint, w/ acceptance tests based around README examples

@ahtcx
Copy link
Contributor

ahtcx commented Dec 20, 2018

What compile errors were you getting?

@Aaronius
Copy link
Owner

Thanks @mike-north, especially for hooking up tests. @paulblyth If you ever have a moment I'd like to get your review on this as well.

@mike-north
Copy link
Contributor Author

What compile errors were you getting?

you had

penpal/src/index.d.ts

Lines 12 to 14 in 8c7d9f0

type ConnectionMethods<T> = {
[P in keyof T]: () => Promise<any>;
};

and are using it in several places without providing a generic argument

interface IConnectionObject<Methods extends ConnectionMethods> {

interface IChildConnectionObject<Methods extends ConnectionMethods> extends IConnectionObject<Methods> {

(and more)

Additionally, the types around parent/child connection methods weren't offering any type safety, or properly taking into account that

{
   add(number, number): number
}

turns into

{
   add(number, number): Promise<number>
}

@mike-north
Copy link
Contributor Author

mike-north commented Dec 20, 2018

Remaining test failures are likely due to forks not having access to your sauce labs credentials (in travis, forks have no environment variables for security reasons). You could probably just open a different PR on a branch in this repo and close this one

types/index.d.ts Outdated
iframe: HTMLIFrameElement;
}

type ConnectionMethods<T = {}> = { [P in keyof T]: () => Promise<any> };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new default type parameter that fixes the compile errors. {} is effectively any non-null, non-undefined value. object may be a more reasonable default, but it would potentially break some consumers and necessitate a major release

@ahtcx
Copy link
Contributor

ahtcx commented Dec 20, 2018

My bad... Nice PR though!

@Aaronius
Copy link
Owner

In the file that gets emitted as lib/index.d.ts, my IDE (Webstorm) is flagging this line:

Promise: typeof Promise;

It says: TS2693: 'Promise' only refers to a type, but is being used as a value here. Is this a cause for concern?

@Aaronius
Copy link
Owner

Oddly, I don't get that flag in types/index.d.ts. I might need to research this one.

@Aaronius
Copy link
Owner

Ah, it's because tsconfig.json isn't in lib. This doesn't concern me.

@mike-north
Copy link
Contributor Author

Ah, it's because tsconfig.json isn't in lib. This doesn't concern me.

typically I like to keep the tsconfig.json in a subfolder unless type-checking or TS compilation is necessary for building the library. Even in that case, it's usually desirable to have a different config for declarations.

@Aaronius Aaronius merged commit 6793006 into Aaronius:master Dec 21, 2018
@Aaronius
Copy link
Owner

Published in 3.0.5.

@Aaronius
Copy link
Owner

Thanks again for putting the work into this.

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.

3 participants