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

Representing extensible JavaScript libraries with non-extensible external modules #857

Closed
yortus opened this issue Oct 8, 2014 · 3 comments · May be fixed by Woodpile37/TypeScript#10
Closed
Labels
Fixed A PR has been merged for this issue

Comments

@yortus
Copy link
Contributor

yortus commented Oct 8, 2014

Extensible JavaScript Libraries use Monkeypatching

I know what the TS spec implies about this question, but it would be great to have a direct response from the TS team regarding this particular JavaScript technique.

It's fairly common to find JavaScript libraries that encourage the practice of API extension through plugins/middleware. jquery has numerous plugins that add properties to its global $ object. Express similarly has middleware that add properties to its Application and Request objects.

These are both examples of monkeypatching. It seems widely accepted, perhaps because plugins/middleware are intentionally chosen for a project, loaded once at initialisation time, and expected to be available throughout the application indefinitely thereafter.

Works Fine with Internal Modules and/or Interfaces

In the jquery example, when JavaScript is loaded via <script> tags, TypeScript has no problem modeling the practice, since internal modules and interfaces are open for declaration-merging. So, jquery.d.ts has:

interface JQueryStatic {
    ... // standard jquery declarations...
}

and jquery.colorpicker.d.ts provides static typing for its monkeypatch as follows:

...
interface JQueryStatic {                   // re-open the interface
   colorpicker: JQueryColorpickerStatic;   // merge in a new member
}

Thus one can conveniently refer to $.colorpicker at compile-time and benefit from typechecking, intellisense, etc.

Regardless what one thinks about monkeypatching, TypeScript models the practice without prejudice.

Doesn't Work with External Modules

The express example is different. express is a CommonJS module, so is naturally modelled in TypeScript as an external module. So express.d.ts declares:

declare module "express" {
    ...
    interface Request ... {
        ...
    }
    ...
}

External modules don't allow declaration-merging, so type declarations for middleware libraries cannot merge new members into the Request interface above. But the middleware libraries certainly do it in practice. Thus, there are fairly common project setups where certain members are known to exist on certain objects, but TypeScript doesn't provide an easy way to model it that way.

One response to this is to say that TypeScript is right and those particular libraries are wrong for using monkeypatching. Once can always write req['isAuthenticated'] instead of req.isAuthenticated when using passport middleware, for example.

Typical Workaround with External Modules

In practice, a workaround is often used. For example in express.d.ts, some declarations are placed outside of the external 'express' module, using internal modules/interfaces that other plugins can merge into. So express.d.ts has:

declare module Express {

    // These open interfaces may be extended in an application-specific manner via declaration merging.
    export interface Request { }
    export interface Response { }
    export interface Application { }
}


declare module "express" {
    ...
    interface Request extends http.ServerRequest, Express.Request {
        ...
    }
}

Note that this is done purely to work around TypeScript's treatment of external modules.

The middleware library express-session provides static typing for its monkeypatch as follows:

declare module Express {          // re-open the module
    export interface Request {    // re-open the interface
        session?: Session;        // merge in a new member
    }
}
declare module "express-session" {
    ... // middleware-specific stuff here...
}

This sort-of achieves the best of both worlds, but there are drawbacks. There is now an ambiently-declared global value Express. Code can refer to it, compile without errors, and fail at runtime (since it doesn't exist). It artificially pollutes the global namespace. And it seems this is an abuse of internal modules that is not officially supported. A variation could be done with just interfaces (no internal module), but that would add even more pollution to the global space of type names.

Any Better Solutions?

I'd like to know of any better solutions. I like that TypeScript just models JavaScript without otherwise being opinionated. In this case I'm not sure. I can't see any technical need for external modules to prevent declaration merging, and whatever the non-technical rationale, it creates a few challenges for accurately typing modular plugin systems. And not just theoratical ones, but very widely used ones like express and others.

As a suggestion, what if just interfaces declared inside external modules were still open to declaration-merging?

@RyanCavanaugh
Copy link
Member

The 1.1 compiler allows declaration merging of ambient external modules; is this sufficient to solve the use cases presented here?

@yortus
Copy link
Contributor Author

yortus commented Oct 8, 2014

Awesome, that fixes this issue and others I can think of too. Has this been documented yet? I haven't come across it until now. I just did a search and came across #280 which covers the same thing.

@RyanCavanaugh
Copy link
Member

I believe the updated spec removed the language stating that merging does not take place across external module boundaries.

@RyanCavanaugh RyanCavanaugh added the Fixed A PR has been merged for this issue label Oct 24, 2014
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants