-
Notifications
You must be signed in to change notification settings - Fork 56
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
Updated types to fix multiple exports error and updated README #17
Conversation
@Aaronius is the TypeScript import of Penpal and the error codes the same as JavaScript? Didn't have time to check the vanilla JS import to see if it was the same, but you're exporting Penpal as default so I expect so? |
Also build failing but I don't know why a change to the types and readme would break it.. 😇 |
README.md
Outdated
|
||
If importing with the error codes you will need to do something like: | ||
|
||
`import { default as Penpal, ERR_CONNECTION_DESTROYED } from 'penpal';` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is your import example here specific to TypeScript users? Typically I would import like this:
import Penpal, { ERR_CONNECTION_DESTROYED } from 'penpal';
That kind of sucks if TypeScript users are forced to do it a different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’ll probably work too actually. Brain must not have been in gear this morning. I can update in GitHub and update my PR but won’t be at my machine to test the TypeScript until tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. The error constants are also statics on the Penpal
object, so I don't really expect consumers to import both Penpal and the constants like you've shown here. They should be able to do:
import Penpal from 'penpal';
console.log(Penpal.ERR_CONNECTION_DESTROYED);
I only export the constants separately in case someone is only using the constants in a particular part of their code and doesn't want to include Penpal in that particular chunk (speaking in Webpack terms). I hope that makes sense. I tried to explain it in the readme:
This provides an opportunity for build optimization (using tools like Webpack or Rollup) in cases where code only needs access to the error constants and not the rest of Penpal.
Anyway, unless users of TypeScript need to do something unique, I'd probably leave this part out. I really should get a sample app with TypeScript working so I can test this stuff out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and in typescript we can do import Penpal, { ERR_CONNECTION_DESTROYED } from 'penpal';
too so I've updated the README
Thanks for helping out @paulblyth. I don't think I understand your question in #17 (comment). It seems like the answer is probably yes. If you could clarify though I could tell you for sure. Yeah, TravisCI and SauceLabs seem to be super finicky. Don't worry about it. |
export default Penpal; | ||
export const ERR_CONNECTION_DESTROYED: Penpal.ERR_CONNECTION_DESTROYED; | ||
export const ERR_CONNECTION_TIMEOUT: Penpal.ERR_CONNECTION_TIMEOUT; | ||
export const ERR_NOT_IN_IFRAME: Penpal.ERR_NOT_IN_IFRAME; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I took a shot in the dark based on some docs I read, but apparently I missed the mark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m still finding my feet. Lots of trial and error until it compiles...
The types is really nice though. If you ever fancy converting it fully to TypeScript I’d be happy to help out (the types come for free!)
Thanks for your help! Released in 3.0.1. |
This should fix #16