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

Proposal: Localized typings for definition files #4668

Closed
jbrantly opened this issue Sep 6, 2015 · 11 comments
Closed

Proposal: Localized typings for definition files #4668

jbrantly opened this issue Sep 6, 2015 · 11 comments
Assignees
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript @types Relates to working with .d.ts files (declaration/definition files) from DefinitelyTyped

Comments

@jbrantly
Copy link

jbrantly commented Sep 6, 2015

This is another proposal for the issue explained in #2839 and #4665.

Problem

Ambient external module declarations are global in nature. For example, if the following is contained in a definition file included in the program...

declare module "myutils" { }

...then myutils can be referenced using an import statement anywhere in the entire program.

In contrast, npm packages can generally only import modules that they have explicitly declared as a dependency or that an ancestor in the dependency graph has declared as a dependency.

This difference can cause issues when multiple copies of (possibly different versions of) definition files are pulled in for the same package. This proposal will use the following example (from #2839) as a reference.

  • myprogram
    • mylib
      • myutils@1.0
    • myotherlib
      • myutils@2.0

Proposal

To solve this, I propose an opt-in way for a definition file to declare that all imports/references contained in the definition file are localized only to that definition file. This would essentially allow definition files to define packages without accidentally leaking types to the global namespace, very similar to how an npm package does not make its dependencies directly available to the dependent package.

Syntactic changes

There would need to be a way for a definition file to say that it is opting-in to this feature. For example:

/// <references localized="true" />

declare module "myLib" { }

I'm not personally tied to what this looks like. If this proposal takes hold, let the bike-shedding begin.

Semantic changes

Any ambient declarations (modules, namespaces, variables, etc) that are referenced by the opting-in definition file are scoped to that file only and not made available to the program at large. This includes anything pulled in either through a /// <reference /> tag or an import statement. Ambient declarations actually made in the definition file are, of course, made available to the requesting scope (whatever that may be).

// myutils.1.0.d.ts

declare namespace MyUtils {
  export interface SomeInterface { }
}

declare module "myutils" {
  export = MyUtils;
}

// myutils.2.0.d.ts

declare module "myutils" {
  export interface RenamedInterface { }
}

// mylib.d.ts

/// <references localized="true" />
/// <reference path="myutils.1.0.d.ts" />

declare module "mylib" {
  import myutils = require('myutils');

  // if using the proposal in #2839, myutils.SomeInterface would not be available
  // but works with this proposal
  export var foo: myutils.SomeInterface;
}

// myotherlib.d.ts

/// <references localized="true" />
/// <reference path="myutils.2.0.d.ts" />

declare module "myotherlib" {
  import myutils = require('myutils');
  export var foo: myutils.RenamedInterface;
}

// myprogram.ts

/// <reference path="mylib.d.ts" />
/// <reference path="myotherlib.d.ts" />

// this will error since neither declaration of the "myutils" module was
// made available globally
import myutils = require('myutils');

// this will also error since the MyUtils namespace was not made
// available globally
var foo: MyUtils.SomeInterface;

Any ambient declarations (modules, namespaces, variables, etc) from a "higher level" are allowed, but will be overridden by any ambient declarations referenced by the definition file. No interface/module/namespace merging will occur.

// mylib.d.ts

/// <references localized="true" />
/// <reference path="myutils.1.0.d.ts" />

declare module "mylib" {
  import myutils = require('myutils');
  export var foo: myutils.SomeInterface;

  // this errors since the 1.0 "myutils" module overwrites
  // any inherited module (is not merged)
  export var bar: myutils.RenamedInterface;
}

// myotherlib.d.ts

/// <references localized="true" />

declare module "myotherlib" {
  // this still works even though there is no <reference /> for
  // it, inherited from higher level
  import myutils = require('myutils');
  export var foo: myutils.RenamedInterface;
}

// myprogram.ts

/// <reference path="myutils.2.0.d.ts" />
/// <reference path="mylib.d.ts" />

import myutils = require('myutils');

// will error, the 2.0 "myutils" module isn't somehow merged with 1.0
export var foo: myutils.SomeInterface;

// works
export var bar: myutils.RenamedInterface;

Other

@jbrantly
Copy link
Author

jbrantly commented Sep 7, 2015

@weswigham @poelstra You've both expressed interest in declarations that don't pollute the global namespace. Do you have any thoughts on this proposal?

@poelstra
Copy link

poelstra commented Sep 7, 2015

I like the idea of allowing references to be forced 'local', but for a different use-case: when I have a test-something.ts, that file is intended to be run with mocha. However, if I currently put a <reference path="mocha.d.ts" /> in it, the it and describe become available in the whole program, which is not what I want. (We had a project where we had both jasmine and mocha, and their typings conflict, so we had to split them into separate directories, with separate tsconfig.json, again separate from the shared code between them.)

However, I would prefer to not have to write these <reference path="..." /> lines anymore. Although they're acceptable for the mocha-case (it's 'magically' available, so a magic reference-line seems ok), it seems redundant to manually specify where the .d.ts file is to be found, when I already do an import foo from "bar"; and can have the compiler search for the typings.

Also, having a /// <references localized="true" /> line in the .d.ts file of a library makes it no longer suitable to use as an 'isomorphic typing' (as @mhegazy called it).

It is indeed a solution for solving the 'accidental globals', but I think #4673 (the 'legacy mode' part of it) solves it more elegantly by basically automatically deciding to apply your 'localized' setting when importing.
And we still need a solution for the 'intentional globals'...

@jbrantly
Copy link
Author

jbrantly commented Sep 7, 2015

@poelstra Thanks for your comments. Let me address each one.

However, if I currently put a in it, the it and describe become available in the whole program, which is not what I want.

That depends entirely on how the definition file is written, I suppose. If it only defined it and describe as part of declare module 'mocha' {} then that wouldn't be an issue. This is a fundamental issue with how most definitions are written today. See #4337

However, I would prefer to not have to write these lines anymore.

Sure, that was just for example purposes here. I would expect ideally that TypeScript would use some magic lookup logic like in your proposal if there was an import, unless you really did want to reference a file (like a sibling definition file with some common types, for example).

Also, having a /// <references localized="true" /> line in the .d.ts file of a library makes it no longer suitable to use as an 'isomorphic typing'

That's not true. The definition file itself can do anything it want (create globals, create ambient modules, whatever). It's only definitions that it references that are restricted. This also relates back to #4337, which is that ideally we shouldn't be writing isomorphic definition files this way anyways. Instead we should be splitting them into multiple files so that program authors can choose how they plan on using it (module or global).

but I think #4673 (the 'legacy mode' part of it) solves it more elegantly by basically automatically deciding to apply your 'localized' setting when importing.

Sort of. What if the definition file that is being imported using the legacy mode uses <reference /> to another definition file that declares a global. Now that global has leaked. This proposal guarantees no leaks down the entire dependency chain. Btw, this isn't just a hypothetical. Yet again, see #4337 😄

And we still need a solution for the 'intentional globals'...

This already solves intentional globals. Definition files that you reference from your program still have the full power that they have today to create globals. It's only dependencies of those definition files (if the definition file so chooses) that would not have the power to create globals, which is a good thing. If your program needs access to a global that's created by a dependency in JS-land, then simply include the definition file for that dependency as well and the global will be made available. Eg:

  • myprogram
    • mylib
      • promiselib // in JS-land, this creates a global, but in TS-land it would not
    • promiselib // if you wanted access to the global in TS-land, simply reference it yourself

@poelstra
Copy link

poelstra commented Sep 7, 2015

That depends entirely on how the definition file is written, I suppose.

No, it and describe are 'real' globals. You don't explicitly import mocha = require("mocha"); or something, they simply are magically available (because you run the file as e.g. mocha test-something).
I don't like this kind of automagic, but that's what we have today. But today, when I add that reference line to make them available for one file in the project, they automatically become available for all files in the project. Your opt-in localization would help with that.

It's only definitions that it references that are restricted.

Hmm, but if a typing makes a 'real' global available (i.e. attaches it to window or global), and I then include it with localized = true, the global basically gets 'degraded' into a local thing in the typings, but it's still a 'real' global in the JS. So it will explode at runtime when you use it as a 'global' (i.e. Promise instead of importedLib.Promise).

Instead we should be splitting them into multiple files

That's a lot of work for the typing authors, isn't it? (Although I agree it'd be nice for us users :))

What if the definition file that is being imported using the legacy mode uses to another definition file that declares a global.

Then all of these will recursively have the same trick applied. No leaking. (Unless one uses the opt-in 'real global escape', in which case it must leak, in my opinion.)

And yes, I ran into the issue you mention in #4337. It regularly happens that I forget to import Promise = require("bluebird");, compiler doesn't complain, runtime explodes :-/

This already solves intentional globals. (...) would not have the power to create globals

They will still create the global at runtime. The fact that you're hiding the typing for it doesn't mean it isn't there. So it will still conflict at runtime.

Suppose there's two (incompatible) packages: global1 and global2 that both make a global foo available, and both myLib1 and myLib2 try to use 'their' foo:

  • myProgram
    • myLib1
      • global1
    • myLib2
      • global2

In your proposal, myLib1 would 'locally' include global1, so compiler won't complain. Same with myLib2, no complaints from compiler. However, at runtime, this still explodes, because the real foo (remember, not myLib1.foo, really just foo) can only be either global1 or global2's foo.

By allowing the 'real global' escape, the compiler can know that the presence of foo from global1 is conflicting with that of global2 (or better: usage of foo in myLib1 and myLib2).

Note: both our proposals suffer from this problem, which I believe can only be 'solved' (well, it can generate a proper compile-time error message) when we additionally implement this explicit global escape.

@jbrantly
Copy link
Author

jbrantly commented Sep 7, 2015

But today, when I add that reference line to make them available for one file in the project, they automatically become available for all files in the project

Ah, I see what you're saying. Interesting.

So it will explode at runtime when you use it as a 'global'

But since it's not available in TS-land as a global, you wouldn't be referencing it as a global.

That's a lot of work for the typing authors

It is, but....

It regularly happens that I forget to import Promise = require("bluebird");, compiler doesn't complain, runtime explodes :-/

This is an area that I'm passionate about since I help maintain the definitions for React. I really want TS to improve in this area.

Then all of these will recursively have the same trick applied. No leaking

I wasn't clear on this point. If that's the case then it's actually a whole lot like this proposal.

They will still create the global at runtime. The fact that you're hiding the typing for it doesn't mean it isn't there. So it will still conflict at runtime.

Again, I think the right approach here is for TS to not complain. It's not a guarantee that the globals will conflict (many of the Promise polyfills are more or less compatible) and it just seems like it would be really hard to somehow tell TS to not spew a bunch of errors in this situation even though you know the runtime won't conflict.

@poelstra
Copy link

poelstra commented Sep 8, 2015

(many of the Promise polyfills are more or less compatible)

Yes, more or less :)

But note the following: #4665 (comment):

E.g. var p = new Promise(...) might still be allowed even if two 'promise polyfills' both declare a Promise, but p.done() fails if only one of them provides that.

i.e., one of them has

// Note: consider this exported and used as 'real' global, i.e. attached to `global`
export interface Promise<T> {
  constructor(...) { ... }
  then(...): Promise<T> { ... }
  done(...): void { ... }
}

and the other one doesn't have the done() method.

All is fine (no compiler errors) as long as you're only using the constructor and then(), but the compiler should probably complain when any of the packages tries to use .done().
Unless you can convince the compiler that the version with the .done() is going to 'win' at runtime (e.g. because you force it to be the first/last script executed).

@jbrantly
Copy link
Author

jbrantly commented Sep 8, 2015

Unless you can convince the compiler that the version with the .done() is going to 'win' at runtime (e.g. because you force it to be the first/last script executed).

That's exactly what I'm saying, and implemented in TS-land by "hiding" all of the globals except for the one you actually directly pull into your program.

@tomwanzek
Copy link

Hi All,

It seems this is an existing issue, related to my issue involving local module augmentation.

I have been working away on typescript definition files for the new modular version 4 of Mike Bostock's famous D3js. I staged the work in progress in a repo for now, as I am experimenting with the ability to type the this context of functions used as callbacks. (I am using typescript@next (v1.9.0-dev.20160622-1.0)).

D3 has a module d3-selection which allows the creation of Selection objects. A separate D3 module, d3-transition, can be loaded which extends the Selection object. In my definitions, this aspect is currently modeled as a module augmentation of the Selection interface.

When 'shape testing' the internal consistency of the definition files, I noticed that the module augmentation bleeds into a typescript test module, which solely imports d3-selection (import * as d3Selection from '../../src/d3-selection';)

A test module which imports both D3 modules, works as expected in the test file have in my local repo. I will push that one shortly.

The issue linked at the beginning of this comment has more detail on the failure mode including a relevant code snippet and links to the test in the repo which shows module augmentation bleed.

While under development, it uses straight import statements relying on node module resolution with relative paths. I recently stripped out typings to focus on the D3 aspect of things and eliminating a possible failure point.

Is there a way to localize the module augmentation at this point? Fair chance, there is another issue/answer tracked on typescript which evaded my attention.

Thanks for all the hard work on typescript and pointing me in the right direction!!!

@mhegazy
Copy link
Contributor

mhegazy commented Jul 4, 2016

When you load a file that contains a module augmentation, the module will be augmented all the time. there is no way to cleave the augmentation from the original declaration. This also matches the runtime behavior, at runtime a module with such side effects, usually will add some new methods on a prototype object from the other module, so any uses of the main module will see it.
i am not familiar with D3 that much to be able to give more helpful comments, but what you describe sounds right to me. if you do not want the augmentation, make sure the containing file is not included in the compilation.

@tomwanzek
Copy link

@mhegazy Thanks, you are of course correct. I chased an edge case wanting to have the cake and eat it by testing the definition shape with and without augmentation in the same repo, without explicitly narrowing the compilation scope. It's all good.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 21, 2016

As noted in #4673 (comment)

The conclusion here was to use npm for distributing declaration files, and use the npm dependency model to resolve conflicts.

Declaration files need to be modules, and not global. for UMD modules they should be using the new export namespace as <id> syntax to export their globals.

Please see more documentation about authoring declaration files and distributing them on npm at https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/declaration%20files/Introduction.md

@mhegazy mhegazy closed this as completed Jul 21, 2016
@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
In Discussion Not yet reached consensus Suggestion An idea for TypeScript @types Relates to working with .d.ts files (declaration/definition files) from DefinitelyTyped
Projects
None yet
Development

No branches or pull requests

6 participants