-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix export default for all colors #86
base: main
Are you sure you want to change the base?
Conversation
index.js
Outdated
} = createColors() | ||
} = colors | ||
|
||
export default colors |
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.
Doesn't that break on systems that do not support ESM imports?
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.
@kibertoad not sure, still need to testing. But I think rollup
will handle that?
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.
Needs testing!
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 remember default exports being a problem even with rollup, that was mentioned e. g. in Webpack project
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.
What you say about the types is true, but it was probably a mistake since I didn't intend to support default exports. Named exports, default exports, or both? Which one is more idiomatic in Node? Should we even have a default export? |
@jorgebucaran Well, if you don't intend to support default exports, you should fix types avoid a mistake for users. Also |
Of course. Can you show us how to fix them? As I said, this was likely a mistake, because I didn't intend to support default exports when I wrote the types. Now, I'm open to discussing adding them in the future. |
Problems
For example if I do this, will get an error:
And then need to change to:
So I'm not sure if there was a special reason for this design? otherwise the general import usage case would be more reasonable and matched the type system.😅