-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
Require Node.js 12.20 and move to ESM #181
Conversation
antongolub
commented
Jul 16, 2021
•
edited
Loading
edited
- add named exports
- update all deps to the latest versions
- add Node.js v16 to test matrix
- apply xo --fix
- BREAKING CHANGE: require Node.js >= 12.20
- BREAKING CHANGE: drop default exports, remove all globby.* methods
BREAKING CHANGE: require Node.js >= 12.20
Thanks for working on this. You need to update index.d.ts (https://github.com/sindresorhus/typescript-definition-style-guide) and the readme for ESM too. |
}; | ||
}; | ||
|
||
module.exports = async (patterns, options) => { | ||
export const globby = async (patterns, options) => { |
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 think there should be the following named exports:
globbyAsync
globbySync
globbyStream
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.
Ok. But let's save globby
as globbyAsync
alias.
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 disagree. Aliases cause confusion.
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.
Maybe just globby
and globbySync
?
BREAKING CHANGES: legacy cjs API was completely removed
rfr |
I can never decide how to handle named exports with async and sync methods:
The former looks nicer, but the latter is clearer. Any opinion? |
My guess is that for most users, the migration will look like this: |
Alright. Let's go with |
@sindresorhus, your turn |
@sindresorhus sorry, why do you avoid the default export here? |
I guess for consistency import? But I think maybe we can export both named and default? |
It's weird to have one default and one named when there are two main exports. I only do default and named export mix when there's only one main export and some secondary exports (like error, or helper utilities). |
No, there should be only one way to import the thing. Aliases create confusion and inconsistency. |