Skip to content

External modules that don't export anything emit empty declarations. #6022

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

Closed
robwormald opened this issue Dec 9, 2015 · 15 comments
Closed
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@robwormald
Copy link

Background - in RxJS we have a number of "operators" that extend our base Observable type.

//src/Observable.ts
class Observable {
  map: <R>(project: (x: T, ix?: number) => R, thisArg?: any) => Observable<R>;
}
// src/operators/map.ts
export function map<T, R>(project: (x: T, ix?: number) => R, thisArg?: any): Observable<R> {
  if (typeof project !== 'function') {
    throw new TypeError('argument is not a function. Are you looking for `mapTo()`?');
  }
  return this.lift(new MapOperator(project, thisArg));
}
//snip

This separation allows a user to import the map operator function and use it without bringing in the other 95 or so - without "side-effects":

import {map} from 'rxjs/operators/map'
map.call(someObservable, (v) => v + 1 );

Without functionBind, this is slightly unergonomic, so we defined a set of "patch" modules that import the Observable type and the the map operator, and patch the prototype there:

//rxjs/add/operators/map.ts
import {Observable} from '../../Observable';
import {map} from '../../operator/map';
Observable.prototype.map = map;

With the idea being a user should be able to simply import 'rxjs/add/operators/map' and have the prototype patched in the background.

Issue:

The emitted rxjs/add/operators/map.d.ts file is emitted completely empty, and so when a user goes to import 'rxjs/add/operators/map.ts', the Typescript compiler errors with

error TS2656: 
Exported external package typings file 'node_modules/rxjs/add/operator/map.d.ts' is not a module. 
Please contact the package author to update the package definition.

Discussed with @mhegazy

CC @Blesh @jeffbcross

RxJS Issue: ReactiveX/rxjs#1010
Sample file: https://github.com/ReactiveX/RxJS/blob/master/src/add/operator/map.ts

@vladima
Copy link
Contributor

vladima commented Jan 16, 2016

fixed in #6213

@vladima vladima closed this as completed Jan 16, 2016
@vladima vladima added the Fixed A PR has been merged for this issue label Jan 16, 2016
@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

@vladima I'm still seeing this issue. When doing:

// a.ts
export class A { }
// b.ts
import {A} from './a';
// do some prototype patching or other side effect stuff here

With the declaration option set to true, the declaration file for b.ts is still generated empty, which makes it impossible to import it for consumers of the library. I'm not sure there's anything that needs to be changed in what is generated, but possibly the error for importing an empty declaration file for its side effects should be removed.

@vladima
Copy link
Contributor

vladima commented Feb 2, 2016

you need to declare module augmentation for ./a in b.ts in order to explicitly tell compiler that this is module is patching its imports (or global scope). In this case compiler will keep treat this file as internal module.

// a.ts
export class A {}
// b/ts
import {A} from "./a";
(<any>A.prototype).foo = function() {}
declare module "./a" {}

Output after running node built/local/tsc.js b.ts -m commonjs -d:

// b.d.ts
declare module "./a" {
}
export {}; // still an external module

@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

@vladima Some modules don't have any type signatures to merge. They simply mutate existing members. A good example is this file, which assigns a function to a static class member declared elsewhere:

import {Observable} from '../../Observable';
import {DeferObservable} from '../../observable/DeferObservable';

Observable.defer = DeferObservable.create;

// export var _void: void; // This is a workaround the RxJS team added

The .d.ts file generated for this is empty, which means consumers of the compiled JS + definitions will get an error when trying to import it, although within the TypeScript codebase it is treated as an external module and can be imported without problems.

@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

Would you recommend adding an empty module declaration for every such file?

import {Observable} from '../../Observable';
import {DeferObservable} from '../../observable/DeferObservable';

Observable.defer = DeferObservable.create;

declare module '../../Observable' { } // Dummy module augmentation to ensure the definition file is not empty

@vladima
Copy link
Contributor

vladima commented Feb 2, 2016

I'm curious if we can just add export {} when emitting declaration file for an external module that does not export anything by itself (just to preserve the fact that original file is external module so it should be ok to import it). pinging @mhegazy @ahejlsberg for their opinion.

@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

Another alternative would be to not emit anything instead of emitting an empty file, since this also eliminates the error (although this might come with its own set of problems that I'm not foreseeing).

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2016

@masaeedu i do not see how you are getting any errors here. here is the setup that i have:

// deferedAugmentation.ts
import {Observable} from '../../Observable';
import {DeferObservable} from '../../observable/DeferObservable';

Observable.defer = DeferObservable.create;

generates:

// deferedAugmentation.d.ts
// Empty

now we use it in anohter module

// anotherModule.ts

import "./deferedAugmentation"; // No Error

Can you give us more information on where you are seeing the error/

@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

@mhegazy Here's what I'm seeing:

image of error in editor

The error (which is identical to the error at the top of this issue) is:

ts/main.ts(1,8): error TS2656: Exported external package typings file 'E:/depot/untracked/test/node_modules/rxjs/add/observable/fromArray.d.ts' is not a module. Please contact the package author to update the package definition.

The corresponding fromArray.d.ts file is empty.

Maybe this occurs only when importing a typings file from a node module? I'm not sure what the actual cause is, so I'm still trying to figure out a way to set up a simplified testcase for you.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2016

aah.. that is a different one, thanks for the info. i think we should relax this check, the error here is not meaningful.

@vladima
Copy link
Contributor

vladima commented Feb 2, 2016

#6846 should cover this

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2016

@masaeedu can you give typescript@next a try later tonight, and let us know if this scenario is working end-to-end or you are still running into issue.

@masaeedu
Copy link
Contributor

masaeedu commented Feb 2, 2016

@mhegazy Will do

@masaeedu
Copy link
Contributor

masaeedu commented Feb 3, 2016

@mhegazy Looks like it works 👍

@benlesh
Copy link

benlesh commented Feb 3, 2016

I know this isn't a helpful comment: But I'm super pleased with the work being done here. Thanks everyone.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

5 participants