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

Require Node.js 12.20 and move to ESM #181

Merged
merged 4 commits into from Jul 22, 2021

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Jul 16, 2021

  • 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
@sindresorhus
Copy link
Owner

sindresorhus commented Jul 17, 2021

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.

gitignore.js Outdated Show resolved Hide resolved
gitignore.js Outdated Show resolved Hide resolved
};
};

module.exports = async (patterns, options) => {
export const globby = async (patterns, options) => {
Copy link
Owner

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

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
BREAKING CHANGES: legacy cjs API was completely removed
bench.js Outdated Show resolved Hide resolved
@antongolub
Copy link
Contributor Author

rfr

@sindresorhus
Copy link
Owner

I can never decide how to handle named exports with async and sync methods:

  • globby and globbySync
  • globbyAsync and globbySync

The former looks nicer, but the latter is clearer. Any opinion?

@antongolub
Copy link
Contributor Author

My guess is that for most users, the migration will look like this:
const globby = require('globby') → import {globbyAsync as globby} from 'globby'.
Well... I'd prefer just import {globby} from 'globby' instead.

@sindresorhus
Copy link
Owner

Alright. Let's go with globby for async and drop globbyAsync.

@antongolub
Copy link
Contributor Author

@sindresorhus, your turn

@sindresorhus sindresorhus changed the title feat: move to esm Require Node.js 12.20 and move to ESM Jul 22, 2021
@sindresorhus sindresorhus merged commit 5c32b4a into sindresorhus:main Jul 22, 2021
@antongolub antongolub deleted the move-to-esm branch July 22, 2021 10:12
@zloirock
Copy link

@sindresorhus sorry, why do you avoid the default export here?

@fisker
Copy link
Collaborator

fisker commented Jul 23, 2021

I guess for consistency import? But I think maybe we can export both named and default? export {globby as default, globby}

@sindresorhus
Copy link
Owner

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

@sindresorhus
Copy link
Owner

But I think maybe we can export both named and default? export {globby as default, globby}

No, there should be only one way to import the thing. Aliases create confusion and inconsistency.

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.

None yet

4 participants