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

Implement named exports #191

Merged
merged 10 commits into from
Aug 7, 2023
Merged

Conversation

bjornstar
Copy link
Contributor

Resolves #137

I added named exports for every method, both is and assert.

While implementing it, I exported validLength and whitespaceString as they were both used internally but seemed useful.

I also couldn't stop myself from improving the type for isInRange to be [number, number] instead of number[]. I can revert this change if you'd like.

I also alphabetized the method and function declarations for easier detection of missing entries.

One nice side effect of this process is that we can remove the method names that were renamed because they were reserved words in typescript. I left them in for backwards compatibility but marked them as deprecated.

I moved the ArrayLike, NodeStream, and Predicate type definitions into the types declaration, they seem to fit in better there. I can move them back if you prefer.

@sindresorhus
Copy link
Owner

This is looking very good.

Can you update the readme usage example to also show named import usage? And also maybe mention that using named import can benefit from tree-shaking.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

One downside of having the default export is be a function is that we cannot re-export all the named exports as a default export, making them tree-shakable even when using the default export. I wonder if it's worth moving the function to something like is.detect(). Thoughts?

I have done something similar here: https://github.com/sindresorhus/query-string/blob/main/index.js

@bjornstar
Copy link
Contributor Author

One downside of having the default export is be a function is that we cannot re-export all the named exports as a default export, making them tree-shakable even when using the default export. I wonder if it's worth moving the function to something like is.detect(). Thoughts?

It makes sense to provide access to the original method through a named export. I had been struggling to find a name for it, detect works for me.

I think it would be nice to allow users to continue using the default export, at least for a little bit to give them time to migrate completely to named exports.

@bjornstar bjornstar requested a review from sindresorhus August 3, 2023 00:21
@bjornstar
Copy link
Contributor Author

I have updated the readme and exported the default method as detect.

readme.md Outdated

```js
import {assertNull, isUndefined} from '@sindresorhus/is';
```
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 this should go at the end of the Usage section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I moved it up underneath the Usage section.

@sindresorhus sindresorhus merged commit 5044c91 into sindresorhus:main Aug 7, 2023
@sindresorhus
Copy link
Owner

Nice work 👍

Do you plan more changes or should I release this?

@bjornstar bjornstar deleted the named-exports branch August 7, 2023 00:51
@bjornstar
Copy link
Contributor Author

Nice work 👍

Do you plan more changes or should I release this?

I'd like to do the is.any / is.all assertion descriptions.

It would also be nice to include dropping node v14 since this is a breaking change, can I reopen #182?

@sindresorhus
Copy link
Owner

I'd like to do the is.any / is.all assertion descriptions.

👍

It would also be nice to include #182 since this is a breaking change, can I reopen #182?

👍

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.

ESM conversion plan: what exports?
2 participants