Use string literals instead of enums

[bjornstar]

There are several good reasons to use string literals instead of string enums in TypeScript:

1. We don't have to repeat ourselves

We define each entry a single time using the word as we want it to appear. We no longer need to write magic boilerplate nonsense like null = 'null'.

2. We don't need to import any enums to use them

We only need to type the word that are using and put quotes around it.

3. The code that we are writing is actually functional JavaScript.

Since it's not just meta programming for the TypeScript compiler, we can write tests that iterate over each entry to ensure coverage.

4. Our types are expanded in intellisense

Instead of : We get the actual entries that we are allowed to use:

I understand it's a fairly big change, so if you like using your enums, that's cool, too.

[bjornstar]

All and all, I support this change. Unless there’s actual uses for the enum outside our implementation (if users use that enum, in JS code especially), the enum can be replaced.

Only thing that bothers me codewise is that it’s a bit unclean (style wise) and the naming seem inconsistent.

@sindresorhus what do you think? Should the enum go or not? I hones don’t know if it’s used outside of our implementation details.

Thank you for your prompt review.

You are absolutely right about the naming. It showed there was a clear problem with my implementation as well. I have modified the tests to ensure that the definition of Primitive and TypeArray are covered.

I have updated my PR and I think it looks better thanks to your comments.

[sindresorhus]

I personally love enums in general, but not in TS. enum in TS is awkward and it's only a compile-time feature, so I agree it makes more sense to use strongly-typed string literals.

[sindresorhus]

Don't force push. It's better to add additional commits. That makes it easier to review what changed. We'll squash when merging anyway.

[bjornstar]

Don't force push. It's better to add additional commits. That makes it easier to review what changed. We'll squash when merging anyway.

Understood. I have gone ahead and split the type names from the types.

I am wondering how you'd prefer to handle the type guards like isPrimitiveTypeName. I prefer using any rather than casting. Personally I find that most of the time that a cast is used, it's turning off TypeScript.

In fact, there's an issue with getObjectType. There's no guarantee that it returns an ObjectTypeName. It could return something that's not on the list. There's even a test which asserts this behavior is expected. How would you like to resolve this?

[bjornstar]

I went ahead and fixed the tests in the way I felt most appropriate.

This PR should be considered a breaking change, but the end result is that the values returned from is() are now type safe. It will only return values from the list of supported typeNames. If that's not what you want, please let me know.

[sindresorhus]

I am wondering how you'd prefer to handle the type guards like isPrimitiveTypeName. I prefer using any rather than casting. Personally I find that most of the time that a cast is used, it's turning off TypeScript.

There's no casting in my last suggestion here: https://github.com/sindresorhus/is/pull/113#discussion_r436247173

[sindresorhus]

In fact, there's an issue with getObjectType. There's no guarantee that it returns an ObjectTypeName. It could return something that's not on the list. There's even a test which asserts this behavior is expected. How would you like to resolve this?

Doesn't really matter does it? It's only used internally and we control the inputs.

[bjornstar]

I am wondering how you'd prefer to handle the type guards like isPrimitiveTypeName. I prefer using any rather than casting. Personally I find that most of the time that a cast is used, it's turning off TypeScript.

There's no casting in my last suggestion here: #113 (comment)

function isPrimitiveTypeName(name: string): name is PrimitiveTypeName {
    return primitiveTypeNames.includes(name);
}

This causes a typescript error when you try to use it with includes: Argument of type 'string' is not assignable to parameter of type '"string" | "number" | "bigint" | "boolean" | "symbol" | "undefined" | "null"'.

I'm not sure I understand the objection to any. The function determines if something (which could be any type) is of a specific type. There's no need to be so strict on the input.

[bjornstar]

In fact, there's an issue with getObjectType. There's no guarantee that it returns an ObjectTypeName. It could return something that's not on the list. There's even a test which asserts this behavior is expected. How would you like to resolve this?

Doesn't really matter does it? It's only used internally and we control the inputs.

It's used as the output from the primary function: is() when the input is an object.

Currently:

is(document.createElement('div'));
//=>HTMLDivElement

HTMLDivElement is not in the list of TypeName enums which means that the type is incorrect. In fact it could be any string when the input is an object.

With these changes:

is(document.createElement('div'));
//=>HTMLElement

[bjornstar]

Is there anything I can do to help move this along?

[sindresorhus]

I'm not sure I understand the objection to any. The function determines if something (which could be any type) is of a specific type. There's no need to be so strict on the input.

I prefer using unknown whenever possible as it’s more type safe. Imagine if in the future, the method does additional things and any then allows something that is not valid. I think the input can remain unknown and just use as any when you need it. That way it’s clear where any is needed even if the function expands in the future.

[sindresorhus]

It's used as the output from the primary function: is() when the input is an object.

I’m tempted to make the common case type-safe. But that would make it inconvenient for people needing other types. Alternatively, we could use LiteralUnion*, but it only helps for autocomplete, not type safety, so not sure it’s worth it. Or we just make the return value a string.

What would you do?

* https://github.com/sindresorhus/type-fest/blob/master/source/literal-union.d.ts

[bjornstar]

It's used as the output from the primary function: is() when the input is an object.

I’m tempted to make the common case type-safe. But that would make it inconvenient for people needing other types. Alternatively, we could use LiteralUnion*, but it only helps for autocomplete, not type safety, so not sure it’s worth it. Or we just make the return value a string.

What would you do?

• https://github.com/sindresorhus/type-fest/blob/master/source/literal-union.d.ts

Well, I already showed you what I would do.

I'll go ahead and remove all the functional changes in this PR and stick to replacing enums with string literals.

[bjornstar]

Are there any changes that need to be made?

[sindresorhus]

@bjornstar Looks great now. Thanks for proposing and working on this. 🙌

Do you plan any more PRs with breaking changes? If not, I'll go ahead and do a major release.

[bjornstar]

@bjornstar Looks great now. Thanks for proposing and working on this. 🙌

Do you plan any more PRs with breaking changes? If not, I'll go ahead and do a major release.

If you want to make it type-safe I'm up for making a new PR.

[sindresorhus]

If you want to make it type-safe I'm up for making a new PR.

That would be awesome 👍