Node module resolution resolving regardless of typings field

[blakeembrey]

I've run into this a bundle of times now and usually just force myself back to using classic module resolution, and looking into the code briefly yields no reason this should be happening.

Basically, I have a lot of modules like popsicle which does not specify a typings field in package.json. However, once I install it using typings (which is just wrapping everything up into ambient module declarations) and use it, it starts to resolve the actual files even though the ambient definitions are already provided. I can only assume that there's some sort of declaration file merging doing on when using node module resolution, but some clarification would be good.

Here's a snippet of the generated definition:

// Compiled using typings@0.5.2
// Source: node_modules/popsicle/dist/form.d.ts
declare module 'popsicle/dist/form' {
import FormData = require('popsicle~form-data');
export default function form(obj: any): FormData;
}

Notice that the file is being correctly rewritten. However, with node resolution enabled, it'll also load the original file too - even though nothing at all references it. This results in the original code generating compiler errors and having to switch back to classic.

node_modules/popsicle/dist/form.d.ts(1,27): error TS2307: Cannot find module 'form-data'.
node_modules/popsicle/dist/jar.d.ts(1,27): error TS2307: Cannot find module 'tough-cookie'.

I can create a replicated repo, if needed, but I hope the issue sheds enough light on the problem.

[vladima]

a self contained repro will definitely be very helpful.

[blakeembrey]

@vladima Created https://github.com/blakeembrey/typescript-6427-repro.

[blakeembrey]

I'm also not sure if I'm running into a different issue, but should export * from '...' also export the default binding? I'm just going to read the ES6 module spec now to figure it out.

Edit: Looks like I have a separate bug to fix, it's correct:

6. If SameValue(exportName, "default") is true, then
  a. Assert: A default export was not explicitly defined by this module.
  b. Throw a SyntaxError exception.
  c. NOTE A default export cannot be provided by an export *.

[vladima]

tl;dr; current behavior is by design.

The first step that compiler is doing is building the list of files that it needs to compile. This list is computed by doing depth first traversal starting from root files ( in your case it will be src/index.ts and typings/main.d.ts ) and following tripleslash references and module specifiers from imports and exports. Note that imports and exports in ambient modules are also processed in the same way since it is a perfectly normal to reference real modules from ambient modules, the only restriction is that module specifier should not be relative name (because it is not clear relative to what base folder we we should resolve this module name). Once this step is finished we assume that we have all files on hands and can move forward to binding and typechecking phases.

In your case typings/main.d.ts has tripleslash reference to popsicle.d.ts and this file contains a bunch of ambient modules that in turn has imports and exports inside. Let's pick

declare module 'popsicle' {
export * from 'popsicle/dist/common';
}

compiler will try to resolve popsicle/dist/common using node resolution rules and this will yield node_modules\popsicle\dist\common.d.ts which in turn also will be processed and bring more files in compilation.

[blakeembrey]

Why does it try to resolve it? It's already been defined as an ambient module. Is there a way around this? It kind of makes it impossible to publish a module that has dependencies on modules that don't have type definitions.

Edit: I'd prefer not to have to hack support.
Edit 2: I can't hack support because I need the name to exist for file-specific imports to work correctly.
Edit 3: It's a pretty common pattern to define ambient modules in the same file (E.g. node.d.ts) and have them import other ambients, are you saying that'll actually break too if a file called fs.d.ts exists?

[vladima]

at the step when compiler collects list of files it in general has no idea whether some module specifier correspond to ambient or real external module (i.e. because you have not yet seen file that will bring this ambient module into scope).

What I can think of off the top of my head is something like this

declare module "popsicle" {
    export * from "~~~popsicle/dist/common";
}

declare module "~~~popsicle/dist/common" {
    export  const get: (options: RequestOptions | string) => Request;    
}

declare module "popsicle/dist/common" {
    export * from "~~~popsicle/dist/common"
}

Every module will effectively be split into two parts:

• one with mangled name - used for internal references in in .d.ts file. All imports and exports in ambient modules use references with manged names.
• one with non-mangled name - used for direct imports in user code, basically only reexports everything from the part with mangled name

[blakeembrey]

@vladima You can't do that (see edit 2) because mangling it means it'll break imports. The point of typings is to allow things like require('react/addons/...'), which mangling it will break.

Edit: It's a common enough pattern to be able to do that and I don't want to break support for it. Even require('xtend/mutable') vs require('xtend/immutable') is a thing.

[vladima]

imports wont be broken since real module react/addons in your d.ts file will be represented by two ambient modules

module "~~~react/addons" {
    // used by imports inside this .d.ts file
}

module "react/addons" {
    // used by user code, reexports everything from "~~~react/addons"
    export * from "~~~react/addons";
}

in user code require("react/addons") will be resolved to the second part so everything should work

[blakeembrey]

@vladima Also, I don't quite understand the first paragraph - can you explain it even more plainly please? Wouldn't it be possible to introduce a restriction where it doesn't look for the file when the corresponding ambient declaration exists in the same file? To find those imports would mean it's already loaded the corresponding ambient declarations anyway, so I don't understand what it means when you say "because you have not yet seen file that will bring this ambient module into scope".

[blakeembrey]

I don't think the prefixing hack is reasonable or even possible. It'll only solve the import from within the ambient declaration itself, but not the users code. Once the user does import 'react/addons' it'll fail if there's a local .d.ts file again since a required dependency might not be typed globally (like the original error message). It's just pushing the problem down a step. I'd really like to preach not resolving definitions that have already been typed globally.

Edit: Replace react/addons with popsicle/dist/form and it's a real problem again.

[vladima]

there are few reasons for current behavior:

• restrictions imposed by current design - in VS we use separate instances of JavaScript engines to for module resolution and semantic analysis and for performance reasons module resolution use scanner to extract information about imports so it will be significantly more complicated to detect ambient modules using only scanner.
• predictability and consistency - final set of files in the project does not depend on order of initial file list. Consider this case:

// file1.ts
import {x} from "some-name";

// file2.ts
declare module "some-name" {
}

If initial list of files was [file1.ts, file2.ts] then during processing of 'file1.ts' when compiler sees import {x} from "some=name" it cannot conclude if 'some-name' refers to ambient or real module so let's say it will pessimistically assume that this can be file and will try to resolve it and probably find something so the final set of files in project will be [ some-name.ts, file1.ts, file2.ts ]. However if initial list of files was [file2.ts, file2.ts] then at the moment when compiler reaches import {x} from 'some-name' it already knows that 'some-name' is ambient module so the final set of files will be '[file2.ts, file1.ts]'.
In theory we can deal with the second issue by deferring import resolution until the point when we process all script files (since only non-external modules can contain ambient modules) but this will make implementation more complicated and but we still have the first issue to solve.

pinging @mhegazy to chime in.

[vladima]

by the way since your package already bundles typings why can't you just use them? I'm not sure that I understand why do you need to repackage separate .d.ts external modules files into one file with a bunch of ambient modules.

[blakeembrey]

@vladima I can't use the built-in bundle because the dependencies don't have their own packaged typings. I believe I've discussed it offline with the TypeScript team before publishing Typings, and I'm available to do so again.

As for the restriction above, I kind of understand - it's a little unfortunate though. Would it be possible to restrict module resolution behaviour when the ambient declaration is within the same file already? For example, node.d.ts doesn't try to resolve imports?

declare module 'events' {
  ...
}

declare module 'fs' {
  import * as events from 'events'
}

[unional]

I have good news that tsc@2.1 seems to have this fixed. 🌷

[blakeembrey]

I'm happy to close this issue. I think it's less of a problem with the ability to now set typesRoot and paths to look up definitions. It'd be a nice-to-have, but not really useful anymore.