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

angular-material: fix declarations to properly augment angular namespace when importing the types via npm #10065

Closed
wants to merge 2 commits into from

Conversation

joeskeen
Copy link
Contributor

@joeskeen joeskeen commented Jul 12, 2016

case 2. Improvement to existing type definition.

  • documentation or source code reference which provides context for the suggested changes.
    • it has been reviewed by a DefinitelyTyped member.

I couldn't get the angular-material typings to work with angular when using TS 2.0 with the @types typings. My code was able to see angular, but not angular.material. I reviewed the new documents in the TypeScript Handbook regarding Definition files and came to the conclusion that since the angular-material library modifies the global angular, the typings for it should follow the pattern of a global plugin (see https://github.com/Microsoft/TypeScript-Handbook/blob/release-2.0/pages/declaration%20files/Library%20Structures.md#global-plugin). After making these changes to the types in my project, I was finally able to get types off of both angular and angular.material.
I haven't been able to verify that the build passes since the Travis build for this branch is broken right now, but I would be happy to verify in any way I can that these changes are valid.

I'm not sure if I'm going about this the right way, but any direction at this point would be helpful :)

P.S. In the DT repo when I am editing the files it doesn't seem to know how to resolve /// <reference types="..." /> references. Is there any way to make that work? (I have my VSCode pointing to my local installation of TS 2.0.)

@joeskeen
Copy link
Contributor Author

joeskeen commented Jul 12, 2016

Update: I noticed the commit history on this branch was turning some angular plugins into this pattern:

/// <reference types="angular" />
import * as ng from 'angular';
declare module 'angular' {
    export namespace material {
        ...
    }
}

I tried using that pattern for angular-material, but it breaks my reference to angular.material. I get the following message:

[ts] 
Module '"~/project/node_modules/@types/angular/index".angular' has no exported member 'material'.
any

I tried a variety of techniques to try to import the angular-material types, but it just doesn't seem to want to merge the declarations to allow both angular.auto and angular.material at the same time. I can only get one to work at a time. The changes in this PR is the only way I could get it to compile.

@joeskeen
Copy link
Contributor Author

@mhegazy do you have any suggestions regarding this case?

@joeskeen
Copy link
Contributor Author

I merged the latest types-2.0 branch into my branch, which fixed my Travis CI build (see https://travis-ci.org/joeskeen/DefinitelyTyped/builds/144311995), but I'm not sure why it didn't trigger the Travis for this PR.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2016

I do not think this is the correct fix. if you are importing angular as a module import * as ng from "angular" then material should be visible on ng.material. if you are using it as a global, then it should be visible as angular.material.

@mhegazy mhegazy added the Revision needed This PR needs code changes before it can be merged. label Jul 13, 2016
@joeskeen
Copy link
Contributor Author

I am using it as a global, but I can't seem to get it using angular.material. It seems that I can only get it to work if importing, but in the project I need to use it as a global. I'll try to post an example someplace illustrating the issue.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 20, 2016

seems to be working fine for me as a global with the current declarations in @types/angular and @types/angular-material. can you share your tsconfig.json, and the version of the compiler you are using?

@joeskeen
Copy link
Contributor Author

I'm closing this. It seems there have been a lot of changes to typings etc. so I'll try my example again and submit a new PR if I still can't get the behavior I need.

@joeskeen joeskeen closed this Jul 26, 2016
@joeskeen joeskeen deleted the types-2.0 branch February 14, 2017 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants