Aaronius / penpal

A promise-based library for securely communicating with iframes via postMessage.
MIT License
389 stars 56 forks source link

Type fixes & acceptance tests #23

Closed mike-north closed 5 years ago

mike-north commented 5 years ago
ahtcx commented 5 years ago

What compile errors were you getting?

Aaronius commented 5 years ago

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 commented 5 years ago

What compile errors were you getting?

you had

https://github.com/Aaronius/penpal/blob/8c7d9f0e98f785caf83f3a84bf4fe061a4cde663/src/index.d.ts#L12-L14

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

https://github.com/Aaronius/penpal/blob/8c7d9f0e98f785caf83f3a84bf4fe061a4cde663/src/index.d.ts#L3 https://github.com/Aaronius/penpal/blob/8c7d9f0e98f785caf83f3a84bf4fe061a4cde663/src/index.d.ts#L8 (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 commented 5 years ago

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

ahtcx commented 5 years ago

My bad... Nice PR though!

Aaronius commented 5 years ago

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 commented 5 years ago

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

Aaronius commented 5 years ago

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

mike-north commented 5 years ago

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 commented 5 years ago

Published in 3.0.5.

Aaronius commented 5 years ago

Thanks again for putting the work into this.