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

Enumerate type names #32

Merged
merged 4 commits into from
Nov 19, 2017

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl commented Nov 10, 2017

This is intended as a discussion starter outlining a possible extended TypeScript usage.
Taking @sindresorhus/is for a first test ride I noticed how is could do more to
provide consumers with type information. I chose TypeScript enum to do so to retain
API compatibility with JS consumers.

  • Enumerate all known type strings in TypeName
  • Use TypeName as return type for is, getObjectType
  • Refactor getObjectType and is to map string results to TypeName
  • Assign the previous string results to their corresponding enum keys to be backwards compatible

Pro

  • More information for TypeScript consumers

Contra

  • Verbosity, verbosity everywhere
  • edit: Bigger API surface

@sindresorhus
Copy link
Owner

Slightly unrelated, but what do you think about this: https://twitter.com/felixfbecker/status/928799529410879488 ?

@marionebl
Copy link
Contributor Author

marionebl commented Nov 10, 2017

type guards as return types look pretty neat, have not used them yet though.

If I understand them correctly they would require is quite a number of types and create a way bigger API surface for TypeScript consumers, right?

source/index.ts Outdated

switch (objectName) {
case 'ArrayBuffer':
return TypeName.ArrayBuffer;

Choose a reason for hiding this comment

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

I don't understand this switch. If the value is "ArrayBuffer", you are returning "ArrayBuffer". The only purpose of it's seems to be to get TypeScript to accept that it's a TypeName, but that's not very idiomatic for TypeScript. Idiomatic is to not have anything in the runtime JS for type reasons - if you only want to work around the type system, use a cast and be done with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you point to a normative source that explains how this approach is not idiomatic / considered a bad practice?

Choose a reason for hiding this comment

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

Yeah, you can extract it from the design goals of the language: https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals

Goals

  1. Impose no runtime overhead on emitted programs.
  1. Emit clean, idiomatic, recognizable JavaScript code.
  1. Use a consistent, fully erasable, structural type system.

Non-Goals

  1. Add or rely on run-time type information in programs, or emit different code based on the results of the type system. Instead, encourage programming patterns that do not require run-time metadata.

Basically, converting a codebase from JS to TS should not change the JavaScript at all. The typings are a build-time construct that gets completely removed and should result in the same JavaScript as before.

@felixfbecker
Copy link

re: type guards. It would not create a bigger API surface at all (in opposite to this PR), it just changes the return type annotation of the specific type checking functions from boolean to a semantic type check return type (that is technically still boolean).

If you are worried about increased API surface, I would rather reconsider this enum addition and only add a union type of all the possible string values (or a const enum that would only be usable in TS and not end up in JS).

@marionebl
Copy link
Contributor Author

marionebl commented Nov 11, 2017

@felixfbecker Thanks for the insights. To be clear: I meant the TypeScript surface as in "things we export in source, consumers import in source". You are right, this is true for the enum, too.

I have not used type guards yet, so you could clarify if this correct:

I understand that to employ type guards on some of the is.* methods you'd have to create some types (and possibly reexport others), e.g. for arrayLike. TypeScript consumers would do things like this then:

import is, {ArrayLike} from '@sindresorhus/is';

function doThings(input: SomeThing | ArrayLike) {
  if (is.arrayLike(input)) {
    // input is known to be ArrayLike
  } else {
    // input is known to be SomeThing
  }
}

@felixfbecker
Copy link

There already is an ArrayLike interface in TypeScript, all the types that is checks for should already exist (globally) as far as I can see

@marionebl
Copy link
Contributor Author

To be honest the jump from TypeScript design goals to your opinion is a non sequitur to me.

I see how casting explicitly helps with readability, so let's do that.

The enum export itself helps TypeScript consumers using is to an extent that is worth the tradeoff of additional JS output in my opinion and def. is an improvement on the current typing to string.

Are you aware of another way to achieve the same level of type information on is() while leaving the JS API untouched?

@felixfbecker
Copy link

Oh yeah, I'm not arguing against adding the enum, I would consider that a feature. I was just talking about the huge switch statement. That said, yeah, you could achieve that level of type information with leaving the JS API untouched:

// Enum whose values will get replaced at build time
export const enum TypeName { null = 'null', undefined = 'undefined' ... }

or

export type TypeName = 'null' | 'undefined' | ...

@marionebl
Copy link
Contributor Author

We are on the same page then :)

@marionebl
Copy link
Contributor Author

@sindresorhus @felixfbecker Could you have another look at this?

@sindresorhus
Copy link
Owner

// @ltetzlaff

@ltetzlaff
Copy link
Contributor

beautiful 👍

@sindresorhus
Copy link
Owner

@marionebl This is not a breaking change for either TS or JS users, right?

@marionebl
Copy link
Contributor Author

@sindresorhus: I think this is breaking for TypeScript consumers, e.g.:

before

import is from '@sindresorhus/is';

function getType(input: any): string {
  return is(input);
}

after

import is, {TypeName} from '@sindresorhus/is';

function getType(input: any): TypeName {
  return is(input);
}

@gioragutt
Copy link
Collaborator

@marionebl breaking - can't use (although I might have understood incorrectly).

@sindresorhus this isn't breaking.

@marionebl
Copy link
Contributor Author

@gioragutt Breaking as in "consumers have to change their code when updating to new version" applies to this change, or am I missing something?

@gioragutt
Copy link
Collaborator

@marionebl tbh, when you mention it, will it actually break if you mark the return type as string and not TypeName?

@sindresorhus sindresorhus merged commit ae3adc9 into sindresorhus:master Nov 19, 2017
@marionebl marionebl deleted the feat/enumerate-type-names branch November 19, 2017 20:11
@felixfbecker
Copy link

As I see it this is not breaking. The enum is a subtype of string. Changing a return type to a subtype of the previous return type is not breaking.

@sindresorhus
Copy link
Owner

@felixfbecker Now that this has landed, would we still benefit from adding type guards? If so, how?

@felixfbecker
Copy link

Yes, definitely! Consider this:

// Without type guards
function foo(param: string | Set<string>): string {
  if (is.string(param)) {
    return param as string // Without the cast, this is a type error
  } else {
    return Array.from(param as Set<string>).join(',') // Without the cast, this is a type error
  }
} 

// With type guards
function foo(param: string | Set<string>): string {
  if (is.string(param)) {
    return param // TS now knows params is a string
  } else {
    return Array.from(param).join(',') // TS now knows params is a Set
  }
} 

@marionebl
Copy link
Contributor Author

@felixfbecker @gioragutt Thanks, TIL.

@sindresorhus sindresorhus mentioned this pull request Nov 20, 2017
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

5 participants