-
-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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. |