ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.7k stars 3k forks source link

Missing Definition file for UMD module #1481

Closed hrajchert closed 8 years ago

hrajchert commented 8 years ago

I've downloaded and compiled the project building for all targets, and noticed that the umd/global distribution is missing the .d.ts definition files.

I've played around a little, and the closest I got was to compile to amd using outFile instead of outDir, that generates a single .d.ts, but its missing the Global name "Rx", for example, its generating

declare module "Subscription" {
    export class Subscription {
        static EMPTY: Subscription;
        isUnsubscribed: boolean;
        constructor(_unsubscribe?: () => void);
        unsubscribe(): void;
        add(subscription: Subscription | Function | void): void;
        remove(subscription: Subscription): void;
    }
    export class UnsubscriptionError extends Error {
        errors: any[];
        constructor(errors: any[]);
    }
}

Where I think it should create

declare module "Rx.Subscription" {

Sadly the project Im building now needs the umd version and I can't get typings :(

david-driscoll commented 8 years ago

There's supposed to be a better way to flatten the results coming in the near future ( see https://github.com/Microsoft/TypeScript/pull/5332 )

If you can get it to build with outDir, it may be possible to post-process the file and shiv the declarations in such that declare module " -> declare module "rxjs/, though that would be nasty and painful.

hrajchert commented 8 years ago

Thanks for the soon reply. I'm no expert in typescript, so I don't fully understand the difference, but when compiling with outDir and module like es6, the declaration files start like this

export { Subject } from './Subject';
export { Observable } from './Observable';
import './add/observable/bindCallback';
import './add/observable/bindNodeCallback';
import './add/observable/combineLatest';
import './add/observable/concat';
...

My first attempt was to create a folder only using the .d.ts files, and made my tsd.d.ts file to point to the Rx.d.ts, but it complaint about the export saying it wasn't part of the compilation process or something like that.

The weird part is that, every module I download with tsd or even RxJs 4.x has declares and this creates imports and exports. I assume its because its meant to be used with a module loader, and that would in turn really use ES6 import (thus providing the Rx namespace implicitly).

But as I said sadly I need to use an umd/global approach for now.

I've also noticed that the umd is created from the cjs using browserify... Is there a tool that converts cjs ts into tsd like ts? In other words, change all the imports/exports with ///reference and declares

kwonoj commented 8 years ago

umd is created from the cjs using browserify

RxJS build doesn't use browserify to create its output. Do you mean your build configuration?

converts cjs ts into tsd like ts?

Would you elaborate bit more? Is this mean using ts source as input and generate some sort of type output? or convert type definition from original into something else?

hrajchert commented 8 years ago

@kwonoj The package.json has a task called build_global that uses make-umd-bundle to create the global/umd dist file, which I think is the one used in the CDN. That tool uses browserify to generate umd code from the cjs one.

The "convertion" between cjs ts into tsd is badly phrased because of my lack of knowledge in the subject. But what I mean is this:

When I use tsd to download the type definition for a non-typescript project, like angular 1, I get a .d.ts file that has a lot of declare keywords, like this (taken from rxjs4 .d.ts):

declare module Rx {

    // Type alias for observables and promises
    export type ObservableOrPromise<T> = IObservable<T> | Observable<T> | Promise<T>;

    export type ArrayLike<T> = Array<T> | { length: number;[index: number]: T; };

    // Type alias for arrays and array like objects
    export type ArrayOrIterable<T> = ArrayLike<T>;

    /**
     * Promise A+
     */
    export interface Promise<T> {
        then<R>(onFulfilled: (value: T) => R|Promise<R>, onRejected: (error: any) => Promise<R>): Promise<R>;
        then<R>(onFulfilled: (value: T) => R|Promise<R>, onRejected?: (error: any) => R): Promise<R>;
    }

    /**
     * Promise A+
     */
    export interface IPromise<T> extends Promise<T> { }

    /**
    * Represents a push-style collection.
    */
    export interface IObservable<T> { }

And if I compile rxjs5 using something like this:

tsc ./typings/main.d.ts ./dist/sherman/src/Rx.ts ./dist/sherman/src/Rx.KitchenSink.ts ./dist/sherman/src/Rx.DOM.ts ./dist/sherman/src/add/observable/of.ts -m amd   --outFile ./dist/sherman/umd.js  --target ES5 -d --diagnostics --pretty --noImplicitAny --suppressImplicitAnyIndexErrors

where I use outFile instead of outDir and the module is amd instead of umd (as umd doesn't support outFile), I get a similar result, but without the Rx namespace.

declare module "Subscription" {
    export class Subscription {
        static EMPTY: Subscription;
        isUnsubscribed: boolean;
        constructor(_unsubscribe?: () => void);
        unsubscribe(): void;
        add(subscription: Subscription | Function | void): void;
        remove(subscription: Subscription): void;
    }
    export class UnsubscriptionError extends Error {
        errors: any[];
        constructor(errors: any[]);
    }
}

The cjs or es6 dist instead has a .d.ts like this:

export { Subject } from './Subject';
export { Observable } from './Observable';
import './add/observable/bindCallback';
import './add/observable/bindNodeCallback';
import './add/observable/combineLatest';
import './add/observable/concat';
...

Where instead of declare, it uses import and export.

My first approach was to use the CDN file and include all the es6 or cjs .d.ts files (mantaining file structure) inside the typings folder, along side other external tools. But that didn't worked and I assume it's because the way I'm working. I'm not using a module loader yet, so I don't do imports in my files, I have a ///reference file, and I think that requires the .d.ts to use files with declare instead of import/export.

kwonoj commented 8 years ago

The package.json has a task called build_global that uses make-umd-bundle to create the global/umd dist file, which I think is the one used in the CDN.

: Oops yes, I oversighted that. apologizes.

hrajchert commented 8 years ago

Thats the one I could use, as I'm not using a module loader any time soon. So far I'm using rxjs version 4 installed with bower and I would like to upgrade to version 5, but I can't if I loose typing

kwonoj commented 8 years ago

I have a ///reference file, and I think that requires the .d.ts to use files with declare instead of import/export.

I see. For external modules, you may not ambient reference to external module definition (/// reference). What tsd provides is ambient type definition while type definitions for external modules are not published as ambient declaration. You may refer some of explanantions at https://github.com/angular/angular/issues/5248#issuecomment-156886060 how type definition is being published. In short, there isn't straight conversion of external type definition into ambient (so far I know, maybe I'm incorrect. Bit off topic note, all of ambient declaration in definitelyTyped what you can get via tsd is all hand-crafted, and it is being deprecated in favor of external definition via typings https://github.com/typings/typings).

It is still issue umd bundle does not have correct type definition though, it's somewhat different to your approach to try load it as ambient way. Honestly I haven't played with umd bundles much so may need to see how to make correct type definitions to be included.

kwonoj commented 8 years ago

Also meanwhile, this https://github.com/angular/angular/issues/5796#issuecomment-195478422 might interest you @hrajchert . It's not exactly same illustration, but somewhat similar. As an immediate workaround, this may help you.

hrajchert commented 8 years ago

@kwonoj Great explanation! I think I understand a little bit more of the problem now. I did not know about typings existance, I was using tsd. My question is, can I use typings without using a module loader? If so, I should be able to use RxJs from CDN and still have type definition from typings, right?

Right now Im not using one, so I don't think I can use something like

import Rx from 'rxjs/Rx';

Not sure what I could take from the second comment you mention, I think it is the result of using the outFile flag, that seems to create ambient definition files.

benlesh commented 8 years ago

@kwonoj can you label this as a bug if it's a bug, or if it's just something that needs work, add PRs Welcome?

kwonoj commented 8 years ago

Yes, will do after couple of try on my end. Should be labeled no later than tomorrow.

kwonoj commented 8 years ago

I'm labeling this related to type output but not as a bug for now, since we never had published type definition for global bundles. Honestly, at this moment I'm not exactly aware how to achieve this with current pipelines, may need some investigations.

@hrajchert , for your questions of importing global bundle and using type definition for other builds - I haven't tried it by my own yet. (Apologizes, since I usually work with moduled build all time). If I have anything, I'll update here.

hrajchert commented 8 years ago

Thanks! I'll try to investigate on my own as well :)

kwonoj commented 8 years ago

It seems there's long pole discussion https://github.com/Microsoft/TypeScript/issues/2568 to allow bundle single definitions might be feasible for this usecases (as well as @david-driscoll 's reference to opened PR), but at this moment it's bit uncertain if this could works via custom bundling for UMD package.

Summarizing, I'm seeing this might be hanging for a while until there's tool chains to support this. I've tried dts-generator (https://github.com/SitePen/dts-generator) but wasn't working as expected.

benlesh commented 8 years ago

@hrajchert after reviewing this issue and a lengthy discussion with @kwonoj, I'm not sure this is an issue with RxJS 5 specifically. We can't find any situations where other libraries are solving this problem. It seems to be that you want TypeScript to behave a particular way, which is something we don't really have any control over.

I'm, of course, open to discuss any proposed solution. But given that we don't have any solutions proposed after this time, and given that it seems to be a feature request for TypeScript, I'm going to close this issue for now.

But please, @hrajchert, if you strongly feel I'm wrong, reopen the issue and state your case. I don't want to send anyone away with a bad taste in the mouth. I just don't think this is something that we can fix, rather it's something will have to come from the TypeScript team.

benlesh commented 8 years ago

attn/ @mhegazy for sanity's sake.

mhegazy commented 8 years ago

I am not sure i understood the issue. @hrajchert in your project, are you consuming RX as a module (i.e. import .. from "rxjs") or as a global variable (i.e.RX.Subscription`)?

hrajchert commented 8 years ago

@mhegazy Currently we use it as a global variable.

@blesh Thanks for revisiting this issue, there is no bad taste in my mouth :P. As I see it, there are two possible solutions, either I take the time (or someone in a similar situation) to change the build process for ambient/"global variable" builds and submit a PR, or I change my company code to start using a module system.

Sadly at this point I don't have the time to do either, so I'll keep using RxJs v4 until I can allocate the time. At that point, I''ll probably do the second option, as I see it's a better solution and a must if we ever want to migrate to Angular2, and for mental sanity as well!

The only reason that I would keep this issue open is to encourage someone that has the time and will to submit a PR, but given the same option, I think most people in a similar situation would choose the "start using a module loader".

Having that said, it's a little bit unfortunate to offer the option of global variable without typings, and reduces the value of that distribution type for a real world app. It's still useful for things like jsbin, though.

Another reason why I'd keep this open is to create an issue in the TypeScript project referencing this. I imagine it's problem that affects every open source project written in typescript that wants to be used with bower for example. And even though the trend is leaning to npm at this point, bower is still a widely used tool (as that was the trend not so long ago).

I'm not very familiar in the inner workings of TypeScript, so I don't know if this is possible, but it would be great if they add an option or tool to export a definition file as ambient or import/export regardless of how they written the project (using import/export or module).

mhegazy commented 8 years ago

Looks like this would be covered by the new UMD support in TS 2.0 (see https://github.com/Microsoft/TypeScript/issues/7125). with this, RxJS would declare that the module can be using as a global RX using export as namespace RX an you can use the same definition file both as a module and as a global is you need to.

ichpuchtli commented 7 years ago

In the meantime I submit a pull-request, using the RxJS 4.x (rx.all.d.ts) type definition file and manually search/replace remove/rename the methods to match RxJS 5 following the migration docs.

steeltomato commented 7 years ago

For anyone still trying to get RxJS 5 to load with typings in an AMD / RequireJS environment, you must do the following:

Now you can import * as Rx from '@reactivex/rxjs' from any .ts file in your project.

lock[bot] commented 6 years ago

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.