angular-redux / ng-redux

Angular bindings for Redux
MIT License
1.16k stars 177 forks source link

remove namespace from index.d.ts #192

Open ionutVizitiu opened 6 years ago

ionutVizitiu commented 6 years ago

this is related to https://github.com/angular-redux/ng-redux/issues/177

AntJanus commented 6 years ago

@ionutVizitiu Alright, so I'm out of my depth here. I think I've merged in changes to index.d.ts a half dozen times in the past year (ok, maybe not that much).

Can you point me to a resource on how the type definitions should be generated and why this change is necessary? I'd really like to learn and have a better way to review this branch.

If anyone else can sign off on this, I'll merge it in but I'd like to know more so I can review this correctly as well.

Andrew-Lahikainen commented 6 years ago

I think this is a good pull request. It's kinda tedious to use a namespace to access all of the interfaces. In order to correctly type something now I have to do:

import ngRedux from 'ng-redux';
...
constructor(private $ngRedux: ngRedux.INgRedux) {}
...

I personally don't like that the value of the default import is a string, but that it's also a namespace for type declarations. I know the angular typings do this too, but still...

For supporting global types, you could do what other angular modules do and declare a namespace on the 'angular' module.

For example, angularjs material: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/angular-material/index.d.ts

So we could do

...
constructor(private $ngRedux: ng.redux.INgRedux) {}
...
no-stack-dub-sack commented 6 years ago

@Andrew-Lahikainen Isn't ng.redux.INgRedux ultimately a more tedious way to access the interfaces? And I think it's less intuitive (unless it's clearly documented, I guess), to expect the ng-redux typings to fall under angular's namespace.

Also, when you need angular types and not the angular library itself (for instance if your controllers are defined as classes in separate files), this wouldn't work as well since you would be destructuring the types you needed, and wouldn't have import * as ng from 'angular';.

You wouldn't be able to destructure ng-redux interfaces directly, you'd just have to descructure redux and then it would be the same as before, and confusing if it was called just redux (unless I'm misunderstanding what you mean, which is very possible!):

import { IController, redux } from 'angular'; // hmmm... without the ng.redux context, this is confusing
import ngRedux from 'ng-redux'; // still need this for accessing connect anyway, right?

export default class MyCtrl implements IController {
  constructor(private $ngRedux: redux.INgRedux) {}
  ...
}

Ultimately, I think the way the PR is currently set up, allowing you to destructure they types you need directly is the most efficient across all use-cases.

import { IController } from 'angular';
import ngRedux, { INgRedux } from 'ng-redux';

export default MyCtrl implements IController {
  constructor(private $ngRedux: INgRedux) {}
  ...
}
Andrew-Lahikainen commented 6 years ago

@no-stack-dub-sack I agree it's more tedious, but my thinking was that it would only be tedious to those people who aren't using modules...

The angular types I use declare ng as a global namespace, so I've never needed to import it with an alias. That would definitely be annoying. Maybe that's specific to my setup though, I'd have to check.

I agree removing the namespace is ideal, but how do you deal with those that aren't using modules? I guess they still just import those interfaces? Do these imports get erased when compiled since they're only importing interfaces? Not an expert on TypeScript :p

no-stack-dub-sack commented 6 years ago

@Andrew-Lahikainen

The angular types I use declare ng as a global namespace, so I've never needed to import it with an alias.

huh... wow. So, news to me - my setup also has ng declared as a global namespace. And here I am importing stuff all over the place... ha! Thanks for pointing that out. To be honest, I was just following suit in a project I'm working on at work, where other devs were importing the types - I'm fairly new to working with AngularJS in TypeScript. This is great to know (and going to remove a bunch of unnecessary imports right now!). This also makes more sense when considering your original comments above.

but how do you deal with those that aren't using modules?

fair point

Do these imports get erased when compiled since they're only importing interfaces?

Yes, if nothing else from the module is used in an expression, and we only use a type imported from that module, nothing else is imported (if that's what your asking), so it is not costly, even though it may seem you're importing the whole module, you're really not. See here.

arutkowski00 commented 6 years ago

Is there any progress? I'd also like to use the destructuring import, it's much more convenient and does not actually include any JS code when only importing types.