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

Declaring typings for modules in subfolders #17945

Closed
sebald opened this issue Aug 21, 2017 · 8 comments
Closed

Declaring typings for modules in subfolders #17945

sebald opened this issue Aug 21, 2017 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@sebald
Copy link

sebald commented Aug 21, 2017

TypeScript Version: 2.4.2

Code
Full source can be found here

// some contents of "index.d.ts"

declare module 'material-ui/Button' {
  export { default } from 'material-ui/Button/Button';
  export * from 'material-ui/Button/Button';
}

declare module 'material-ui/Button/Button' {
  import { ButtonBaseProps } from 'material-ui/internal/ButtonBase';

  export interface ButtonProps extends ButtonBaseProps {
    color?: MaterialUI.PropTypes.Color | 'contrast';
    component?: React.ReactNode;
    dense?: boolean;
    disabled?: boolean;
    disableFocusRipple?: boolean;
    disableRipple?: boolean;
    fab?: boolean;
    href?: string;
    raised?: boolean;
    type?: string;
  }

  export default class Button extends MaterialUI.Component<ButtonProps> {}
}

Expected behavior: Defining all modules exported by a npm-module (including sub-modules, like material-ui/Button/Button/) will allow the developer to import not only the root but also the sub-modules.

Actual behavior: TypeScript seems not to pick up sub-modules defined in the "root" index.d.ts, when a module other than the root material-ui is used and expects an index.d.ts/typings definition inside the corresponding folder.

This behavior only occurs if the typings are used inside an npm-module. Using the (linked) typigns locally or inside @types/material-ui works fine.

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Aug 22, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

Seems to be working fine for me locally. what am i missing?
animation

@vyrotek
Copy link

vyrotek commented Aug 22, 2017

Hi @mhegazy I believe you're referencing the pre-beta (0.18) definitions on @types/material-ui.

I think @sebald is trying to provide a newer definition inside the beta npm package "material-ui": "1.0.0-beta.6" itself. Which when imported ends up located in "\node_modules\material-ui\index.d.ts" but doesn't seem to be picked up in VSCode.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

do you have a self contained repro i can look at?

@sebald
Copy link
Author

sebald commented Aug 22, 2017

@mhegazy Sure, here you go: https://github.com/sebald/mui-typings-error

You can find the issue here:
https://github.com/sebald/mui-typings-error/blob/bc4a535ad1066a913c6529ce0ac7eec9aaf5c3aa/src/App.tsx#L2-L10


I tried to resolve the issue yesterday by structuring the typings differently, but had no luck. Basically, importing from the "root" module works:

// tslint:disable:max-line-length
import * as React from 'react';
- import Button from 'material-ui/Button/Button';
+ import { Button } from 'material-ui';

const App = () => <Button>Click me!</Button>;
export default App;

My guess is that TS is using the (correct) modules resolution inside the node_modules. TS only tries to find a Button.d.ts file and doesn't respect the typings defined via declare module 'material-ui/Button/Button' inside the root index.d.ts.

As said, moving the exact same file to @types/material-ui will make the typings work. Same goes for using the file locally (=hosted as a sibling or child of tsconfig.json) 🙂

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

I think i understand the issue, but let me restate it to make sure we are on the same page.

  • you do not want to use @types/material-ui, instead you want material-ui to carry its own types.
  • you want the types to all be in ambient module declarations, i.e. declare module "material-ui/subfolder" {.. instead of having them as modules (i.e. an actual file called material-ui/subfolder/index.d.ts or material-ui/subfolder.d.ts).
  • you also do not want to add a "paths" mapping rule in your tsconfig.json.

if this is accurate, then the behavior your seeing is what i would expect. @types is included by default in your project, and since @types/material-ui is not there, then it is not.
Ambient module declarations, i.e. declare module "material-ui/subfolder" {.. are global scope declarations, but since there is no file with that name, you need to include it in your project directly. either

  1. adding it to the list of includes/files in your tsconfig.json,
  2. add a "paths" : { "material-ui/* : [ "node_modules/matrial-ui/index.d.ts"] }` entry to your tsconfig.json,
  3. adding /// <reference types="material-ui" /> in one of your files,
  4. adding "types": [ "material-ui" ] to your tsconfig.json (recommended).

Having said that, i would recommend against putting types in global space. global space declarations do conflict if there are multiple of them (e.g. multiple version of material-ui due to transitive dependencies), and the compiler will error in these cases. Modules on the other hand have their own scope, so you can have multiple versions of a module co-existing side by-side. so you are doing your users a favor by having your files organized as modules.

Hope that helps.

@mhegazy mhegazy added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Needs More Info The issue still hasn't been fully clarified labels Aug 22, 2017
@sebald
Copy link
Author

sebald commented Aug 22, 2017

Thanks again for your detailed answer! I was a bit confused why TS handles local and typings inside @types different than those provided by the module itself. I only have some follow up questions, maybe you can elaborate on this:

Ambient module declarations, i.e. declare module "material-ui/subfolder" {.. are global scope declarations, but since there is no file with that name, you need to include it in your project directly.

What do you mean by "no file with that name". The actual implementation exists on the corresponding paths. Meaning there is an material-ui/subfolder/index.js for declare module 'material-ui/subfolder' {}`.

So, in order to don't have them in the global scope (which is also done when using @types, correct?), there should be *.d.ts files for the source files?

Modules on the other hand have their own scope, so you can have multiple versions of a module co-existing side by-side. so you are doing your users a favor by having your files organized as modules.

I thought that publishing the types with the npm package is the preferd solution. As a consumer of mostly non-TS libs I fully support this, especially because the typings actually match the library and are not out of date.

Also, having @types that all rely on @types/react often yields some inconsistencies to, because they rely on different versions of React's typing :(

The v1 of material-ui uses flow-type (sadly! 😉), so adding .d.ts files to the sources directly was rejected as first. What is your suggestions? Should the typings just be published via @types then? Or is there a way to generate .d.ts files (as modules) from a single index.d.ts?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2017

What do you mean by "no file with that name". The actual implementation exists on the corresponding paths. Meaning there is an material-ui/subfolder/index.js for declare module'material-ui/subfolder' {}`.

there is .d.ts/.ts/.tsx file

I thought that publishing the types with the npm package is the preferd solution. As a consumer of mostly non-TS libs I fully support this, especially because the typings actually match the library and are not out of date.

yes. what i said before does not contradict with this statement.
If you observe what the compiler behavior when used with --declaration is it creates a .d.ts file for every .js file it creates. the mapping is important in the case of modules, since modules have their own scope.

Also, having @types that all rely on @types/react often yields some inconsistencies to, because they rely on different versions of React's typing :(

The only thing special about @types is they are included into your compilation by default. you could opt out of this by settign "types": [] in your tsconfig.json, or you could add other ones.

The v1 of material-ui uses flow-type (sadly! 😉), so adding .d.ts files to the sources directly was rejected as first. What is your suggestions? Should the typings just be published via @types then? Or is there a way to generate .d.ts files (as modules) from a single index.d.ts?

I see there are .flow files in each folder, seems reasonable to allow a .d.ts as well..

I think the best work around at the moment, is to ask users to add "types": [ "material-ui"].

@sebald
Copy link
Author

sebald commented Aug 22, 2017

Thank you so much for your help! Even though I don't like to have users add types for the lib to their configs, this really seems to be the solution for now. Maybe we'll add .d.ts files alongside the source in the future :)

Again. Thanks! Really appreciate it \o/

I am closing this, since from my side everything is resolved.

@sebald sebald closed this as completed Aug 22, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants