-
-
Notifications
You must be signed in to change notification settings - Fork 83
Maybe fix types #73
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
Maybe fix types #73
Conversation
@@ -1,4 +1,4 @@ | |||
import wsPlugin, { SocketStream } from '../..'; | |||
import * as wsPlugin from '../..'; |
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.
@Ethan-Arrowood @mfpopa @fox1t can you please explain this to me?
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'll defer to @fox1t as he has a better understanding of this than I do
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 checked out this branch and tested with both esModuleInterop
set to true and false (it's false by default). I see find no issues.
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.
To clarify, when using "esModuleInterop": false
(the default), import needs to be done like this import * as wsPlugin from '../..';
. When using "esModuleInterop": true
, import needs to be done like this import wsPlugin from '../..';
.
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.
It works because export = syntax exports a so called namespace that is then imported by import *. Export default has to be used only in presence of .default prop on the exported object.
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.
Yup, does work now!
These changes work for me. I tested with |
We need to standardize import checks in our typescript tests. Different packages are exporting types in different ways, even if the JS code is the same. :/ |
Yes we should |
Fixes #72
@TriangularCube can you please check if this works?