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

refactor(interfaces): replace type aliases #3045

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Nov 4, 2017

Description:

Regarding this issue - that was opened in the TypeScript repo - there is a simple resolution: replace the type aliases with interfaces.

The TypeScript documentation for advanced types includes these comments in the Type Aliases section:

One difference is that interfaces create a new name that is used everywhere. Type aliases don’t create a new name — for instance, error messages won’t use the alias name. [...]

A second more important difference is that type aliases cannot be extended or implemented from (nor can they extend/implement other types). Because an ideal property of software is being open to extension, you should always use an interface over a type alias if possible.

I can see that the issue has been labelled as a bug, but the referenced documentation suggests otherwise.

cc/ @jayphelps @benlesh

Related issue (if exists): microsoft/TypeScript#19198

@rxjs-bot
Copy link

rxjs-bot commented Nov 4, 2017

Messages
📖

CJS: 1377.0KB, global: 745.2KB (gzipped: 118.5KB), min: 146.0KB (gzipped: 31.5KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.204% when pulling 074f95e on cartant:type-aliases into cff9dfa on ReactiveX:master.

@jayphelps
Copy link
Member

Seems like a good solution at first glance.

@cartant
Copy link
Collaborator Author

cartant commented Nov 8, 2017

TIL: documentation can be out-of-date.

Anyway, with the interfaces, you could have it working now, instead of in January (maybe) with TS 2.7.

@jayphelps
Copy link
Member

Totes. Btw rereading my last comment it might have seemed like I meant “at first glance” to mean that it’s not a good solution after all. Whoops! Not what I meant. I just meant that I haven’t tested it or seen if it has any other impacts in real usage. Someone on the core team will review hopefully.

@cartant
Copy link
Collaborator Author

cartant commented Nov 8, 2017

No worries. I took it the way you intended it.

@jayphelps
Copy link
Member

jayphelps commented Nov 8, 2017

I started to think this would be a breaking change because OperatorFunction is exposed publicly, but unfortunately :trollface: fortunately because TypeScript is structurally typed instead of nominally, it doesn't seem to care:

Playground Example

// as an alias
type UnaryFunction_alias<T, R> = (source: T) => R;
type OperatorFunction_alias<T, R> = UnaryFunction_alias<Observable<T>, Observable<R>>;

// as an interface
interface UnaryFunction_interface<T, R> { (source: T): R; }
interface OperatorFunction_interface<T, R> extends UnaryFunction_interface<Observable<T>, Observable<R>> { }

declare let usingAlias: OperatorFunction_alias<void, void>;
declare let usingInterface: OperatorFunction_interface<void, void>;

// no type error
usingAlias = usingInterface;

It would have been very unlikely anyone depended on it anyway.

@benlesh
Copy link
Member

benlesh commented Nov 8, 2017

Interestingly, this is how I first implemented this when working on it, but switched it because I thought it was more readable for other contributors. (interfaces for functions is pretty weird, IMO, but probably a personal preference thing)

If this solves a problem that's blocking you @jayphelps or the documentation efforts in general, I'll happily merge.

@benlesh
Copy link
Member

benlesh commented Nov 8, 2017

Also, if @jayphelps says this solves a problem for our docs, we should add a test to make sure we don't break it in the future.

@cartant
Copy link
Collaborator Author

cartant commented Nov 8, 2017

This is another one that could use a snippet test - to ensure the inferred type is what's expected.

@benlesh benlesh merged commit a968783 into ReactiveX:master Nov 27, 2017
@cartant cartant deleted the type-aliases branch March 31, 2018 02:28
@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants