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

Module augmentation does not work on un-imported node packages / generated d.ts files #8113

Closed
theoy opened this issue Apr 15, 2016 · 7 comments · Fixed by #8200
Closed

Module augmentation does not work on un-imported node packages / generated d.ts files #8113

theoy opened this issue Apr 15, 2016 · 7 comments · Fixed by #8200
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@theoy
Copy link

theoy commented Apr 15, 2016

TypeScript Version: v1.8.10

  1. Create a basic external npm package 'node_modules/ext'. In it, set up the following 2 files:

    // ext.d.ts
    declare var _default: {};
    export default _default;
    
    // package.json (edit it)
      "main": "ext",
      "typings": "ext",
    
  2. Reference package 'ext' from A.ts, using --declaration, generating A.d.ts

    // A.ts
    import ext from 'ext';
    
    declare module 'ext' { }
    
  3. Copy A.d.ts to another directory, that has the ext package installed in node_modules
    // B.ts import * as modA from './A'

Expected behavior:
Generated A.d.ts causes no errors because typed package 'ext' was in the correct location.

Actual behavior:
A.d.ts(1,16): error TS2664: Invalid module name in augmentation, module 'ext' cannot be found.

Workarounds:

  • explicitly import 'ext' module while in B
  • re-export something from B as part of A (triggering an import to be generated in A.d.ts)

Remarks:
This is frustrating because the fact that A depended on 'ext' was an implementation detail, and in reality npm was the one that brought 'ext' into node_modules.

Thoughts?

@DanielRosenwasser
Copy link
Member

Are you using --outFile when you try this out?

@theoy
Copy link
Author

theoy commented Apr 16, 2016

Nope, generating and publishing loose *.d.ts files. 😄

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 16, 2016

I'm not able to repro the issue. Can you upload your code to GitHub or somewhere so I can clone it and take a look?

theoy added a commit to theoy/tsc-8113-repro that referenced this issue Apr 16, 2016
To repro:

1. git clean -xdf
2. cd ext && npm install
3. cd ../A && npm install
4. cd ../B && npm install

Note, TSC is set as npm prepublish, which runs it automatically after npm install.
@theoy
Copy link
Author

theoy commented Apr 16, 2016

@DanielRosenwasser - sure, I published a repo as https://github.com/theoy/tsc-8113-repro

To repro:

1. git clean -xdf
2. cd ext && npm install
3. cd ../A && npm install
4. cd ../B && npm install

There's a comment in B.ts that demos the workaround of importing 'ext' from B. In my real project, 'ext' is an implementation detail, and would prefer to not have to expose it, but was augmenting 'ext' from within A, in order to clean up an aspect of A's implementation.

@vladima
Copy link
Contributor

vladima commented Apr 16, 2016

I can see what is going on. The issue is that in A.d.ts we record the fact that it contains augmentation for module ext but import itself it elided since module does not expose anything from module ext on its public surface.

// A.d.ts
declare module 'ext' {
}
declare var _default: {};
export default _default;

Now if module B tries to consume module A the compiler will find augmentation for ext but won't find anything that imports or exports ext and as a result ext won't be included in the program. This was a deliberate design decision - module augmentations don't add new files in the program, instead they should only work if something else in the program uses module-that-should-be-augmented. However I think that we should relax the rule regarding errors: if we've found an augmentation for module we should check if name in augmentation can be successfully resolved. If yes - we should proceed and don't report any errors (yes, target module might not be imported so nobody will observe the result of augmentation - not a big deal). However if resolution for module name has failed - then we should error.// cc @mhegazy

@theoy
Copy link
Author

theoy commented Apr 16, 2016

@vladima - yep, I figured that's what was going on, especially once exporting in A something from ext resolved the problem. I personally agree that your suggestion sounds right (allow module augments to refer to modules that haven't been imported by other ways). Either that, or detect that the augmentation isn't used in anything exported.

@vladima
Copy link
Contributor

vladima commented Apr 17, 2016

another solution can be: since we now are distinguishing root files from imported we can only issue module not found errors for augmentations defined in root files. The reason is: it is quite possible that some 3rd party module A that was imported in application also defines an augmentation for some another module B but application itself does not use B and as a consequence does not have dependency on it so resolution for B always will fail and user will always see some errors that he cannot get rid of.

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Apr 18, 2016
@mhegazy mhegazy added this to the TypeScript 2.0 milestone Apr 18, 2016
@mhegazy mhegazy added the Committed The team has roadmapped this issue label Apr 18, 2016
@vladima vladima added the Fixed A PR has been merged for this issue label Apr 20, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants