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

Fix export default for all colors #86

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rwu823
Copy link

@rwu823 rwu823 commented Oct 11, 2021

Problems

For example if I do this, will get an error:

import c from 'colorette'

// types safe but get the error in runtime.
c.cyan('hello') // TypeError: Cannot read properties of undefined (reading 'cyan')

And then need to change to:

import * as c from 'colorette'

c.cyan('hello')

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.😅

index.js Outdated
} = createColors()
} = colors

export default colors
Copy link
Collaborator

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?

Copy link
Author

@rwu823 rwu823 Oct 11, 2021

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs testing!

Copy link
Collaborator

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

Copy link
Author

Choose a reason for hiding this comment

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

rollup will transform output to cjs version. So there should be no problem here is about "systems that do not support ESM imports"

image

And I tried that import or require from output file as es5/es6/ts they are all worked.

@jorgebucaran
Copy link
Owner

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?

@rwu823
Copy link
Author

rwu823 commented Oct 12, 2021

@jorgebucaran Well, if you don't intend to support default exports, you should fix types avoid a mistake for users.

Also chalk did export default if this project expects as a alternative.

@jorgebucaran
Copy link
Owner

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.

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