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

remove namespace from index.d.ts #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

remove namespace from index.d.ts #192

wants to merge 1 commit into from

Conversation

ionutVizitiu
Copy link

this is related to #177

@AntJanus
Copy link
Collaborator

@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
Copy link
Contributor

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
Copy link
Contributor

no-stack-dub-sack commented May 12, 2018

@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
Copy link
Contributor

@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
Copy link
Contributor

no-stack-dub-sack commented May 23, 2018

@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
Copy link
Contributor

arutkowski00 commented Aug 29, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants