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

Type generation for "modularly designed" libraries is hard #5269

Closed
benlesh opened this issue Oct 15, 2015 · 17 comments · Fixed by #6213
Closed

Type generation for "modularly designed" libraries is hard #5269

benlesh opened this issue Oct 15, 2015 · 17 comments · Fixed by #6213
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript

Comments

@benlesh
Copy link

benlesh commented Oct 15, 2015

I'm reaching out to you because over at ReactiveX/RxJS we've been using TypeScript, primarily as a means of generating "correct" .d.ts output for our consumers. However, because of the design of our library, that dream isn't really being realized, because of issues outlined here.

The specific problem is that we have a need to export a class with type information about what methods it could have. We end up jumping through some strange hoops to make this happen. The reason the architecture of RxJS 5 works this way is simple: not every Rx project needs every Rx operator, and most project build systems don't have granular tree-shaking to remove all of the RxJS operators that a project isn't using. The "low tech" solution is to just provide a plain Observable object to people and let them import operators and decorate the prototype on the one they use.

I think TypeScript has had some real benefits for us, so I don't want to walk away from it if it still makes sense. But if the primary reason we were using it was to generate .d.ts files, and we end up jumping through hoops to generate those .d.ts files, which cause us to have to workaround TSC complaints, we (at RxJS) should at least re-examine our approach.

I'd really appreciate experts from your community reviewing the issues we've had and offering up suggestions. Ideally, the RxJS project's woes can offer up to you an opportunity to make the TypeScript experience more intuitive in situations like this.

Thanks for your time.

@ahejlsberg
Copy link
Member

We'd love to help solve your problem. First off, let me ask what you would do differently in just plain ES6? In curious if there's something the type system is preventing you from doing, or whether the friction stems from the way ES6 modules work? For example, some would argue that it is an anti-pattern for module imports to have side effects on objects defined in other modules, such as the Observable prototype, but perhaps that's the only meaningful way to implement the kind of modularity you need. TypeScript currently supports type merging across namespace (formerly "internal module") boundaries, but we haven't gone there with ES6 modules because of the side effect smell. Is it something we should consider?

@ahejlsberg
Copy link
Member

BTW, I should add that the pattern of having a library add members to other types (or have the illusion of doing so) is precisely why we introduced extension methods in C#. But in C# it is purely syntactic sugar over rewriting instance method calls into static methods calls, passing the receiver object as the first argument (i.e. obj.foo(...) gets rewritten to Library.foo(obj, ...)). Unfortunately this likely isn't an option for TypeScript as it doesn't really have a counterpart in JavaScript.

@benlesh
Copy link
Author

benlesh commented Oct 15, 2015

First off, let me ask what you would do differently in just plain ES6?

Honestly, not much. There would be less "adding method stubs" (per the issue I linked), and less "casting", I suppose. It might just be easier to maintain that code and a manually written .d.ts file. I was initially worried about keeping those two things in sync, however, outputting types our users want from our build has proved challenging.

curious if there's something the type system is preventing you from doing

There's two things I'm running into. I can't output type information for methods that could optionally exist on the class. ... however, I can add property "stubs" for those methods. ... BUT... If I do that, then any subclass of that class has to implement those as instance properties... but I don't want instance properties because I want those function references on the prototype, not the instance.

class Foo {
   someMethod: () => void
}

// this doesn't work
class Sad extends Foo {
  someMethod(): void {
  }
}

// this works, but sucks, because I'm making a new function copy per instance
class Better extends Foo {
  someMethod = function() {
  }
}

// this works in a more desirable way, but sucks because it's ugly
class Betterer extends Foo {
}
Betterer.prototype.someMethod = function() {
}

Ideally, the first subclass Sad would "just work", because there doesn't seem to be a reason in a prototypal structure, that a type system should differentiate between an undefined instance property and a property on the prototype with a function in it (which is what a class method is). I can see why it would complain if the superclass defined an instance method and the subclass defined a method on the prototype, but if it's undefined in the superclass it doesn't seem like it should be an issue. I'm clearly just putting it there for the type information.

Unfortunately this likely isn't an option for TypeScript as it doesn't really have a counterpart in JavaScript.

The closest thing I can think of is ES7 function bind. But you'd have to add some additional syntax in TypeScript to let the compiler "know" what a standalone function could "bind" too. Like:

extension function add<T as string|number>(x: T) : T {
  return this + x;
}

So if I have a string foo and I type foo., VSCode could know to show me add(x: string): string as an option.

I'm spitballing. Designing languages is hard, I wouldn't pretend to know the edge cases around something like that.

It's probably a lot to commit to for a Stage 0 proposal feature... but if ES7 function bind doesn't make it into JavaScript I'll eat my hat.

@benlesh
Copy link
Author

benlesh commented Oct 15, 2015

Thanks again to you and @RyanCavanaugh for your time. I really appreciate it.

@RyanCavanaugh
Copy link
Member

I can't output type information for methods that could optionally exist on the class

Does this do what you'd like?

interface MaybeMethods {
    someMethod?(): void;
}

class Foo implements MaybeMethods {
    // some core logic, perhaps
}

// All is good
class Sad extends Foo {
  someMethod(): void {
  }
}

@benlesh
Copy link
Author

benlesh commented Oct 15, 2015

@RyanCavanaugh I don't think the Foo.d.ts output would reflect the possibility of a someMethod on Foo though... I think it will say "well, it could be there, but it wasn't so no need to put it in the .d.ts". I guess I could add stubbed methods on Foo maybe?

class Foo implements MaybeMethods {
   someMethod() { throw new Error('not implemented'); }
}

It's just a lot of extra stuff for what we need to do. Which is why I'm leaning toward manually creating .d.ts files and going to plain ES6. That comes with it's own drawbacks though. :\ It's a bit of a pickle.

@ahejlsberg
Copy link
Member

In TypeScript 1.6 we implemented the ability to merge class and interface declarations (see #3332) so extension libraries can add methods to base libraries. We restricted this capability to ambient classes only, but perhaps we should relax that restriction (there wasn't a strong reason for the restriction in the first place). This would allow you to do the following:

// Optional methods
interface Foo {
   someMethod(): void;
   anotherMethod(): void;
   yetAnotherMethod(): void;
}

// Actual implementation
class Foo {
    regularMethod(): void {
    }
}

class Better extends Foo {
    someMethod(): void {
    }
}

It actually works right now, except you get errors on the two declarations of Foo stating that only ambient classes and interfaces can merge. But other than that, everything works the way you'd expect and Foo has the merged set of methods.

@benlesh
Copy link
Author

benlesh commented Oct 16, 2015

That sounds like a decent solution, actually.

@ahejlsberg
Copy link
Member

@mhegazy Opinions?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 16, 2015

Here is the change to stop reporting the errors: #5290; I think we also need a way to allow modules to modify global types (something along the lines of #4166).

@mhegazy
Copy link
Contributor

mhegazy commented Oct 16, 2015

This change should be in typescript@next (1.7.0-dev.20151017) later tonight

@benlesh
Copy link
Author

benlesh commented Oct 17, 2015

👍

@ahejlsberg
Copy link
Member

Longer term we'd also like to fix the deeper problem of not being able to extend types in other modules. There's some information on our thinking in the design notes, but basically we'd like to get to something like this:

// module myoperator.ts
import { Observable } from "../observable";

declare module "../observable" {
    // Extend the Observable<T> interface
    interface Observable<T> {
        myOperator(...): Observable<T>;
    }
}

function myOperator(...) {
    // Implementation of operator
}

Observable.prototype.myOperator = myOperator;

So, when the "myoperator" module is imported it automatically extends the Observable<T> type and the Observable prototype.

Hope this makes sense.

@DanielRosenwasser
Copy link
Member

Should be 1.8.0-dev.20151017.

@benlesh
Copy link
Author

benlesh commented Oct 17, 2015

@DanielRosenwasser ... What is in that tagged version?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 17, 2015

just to clarify, we have forked the release 1.7 into its own branch, and now the version of master, and thus of the nightly builds moved to 1.8. the version of the first nightly (typescript@next) with #5290 is 1.8.0-dev.20151017. TS 1.7 will have the change as well.

@mhegazy mhegazy added Suggestion An idea for TypeScript Committed The team has roadmapped this issue labels Nov 13, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Nov 13, 2015

As mentioned in #5292, modules should be able to add declarations in other modules as well as the global scope. We will need a different issue to track the design and implementation of that, but keeping this open for now.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants