microsoft/TypeScript

TS2345 error is incompatible with how NPM 3 implements side-by-side versioning

octogonz opened this issue · 7 comments

octogonz commented

TypeScript Version:

2.0.3

Actual behavior:

This issue is a continuation of an earlier issue involving symlinks (#8346).

Suppose that library-a is an NPM package that exports a TypeScript class called MyClass, and two other packages consume this class. If those packages find the MyClass.d.ts definition via two different file paths on disk, and if MyClass contains a private member, then the compiler reports an error because it does not trust that the implementations are interchangeable.

This makes sense in general, however there are everyday NPM usage scenarios where provably equivalent definitions must unavoidably have different file paths. In this situation the TypeScript error is not reasonable and makes life difficult for developers.

Code

Unlike the earlier issue #8346, this repro does not involve symlinks at all. In the attached CompilerBug.tar.gz, the different file paths arise as follows:

  • library-a 1.0 exports MyClass

  • library-d exports MyLibrary like this:

    import { MyClass } from '@local/library-a';

export default class MyLibrary {
 public static getMyClass(): MyClass {
   return undefined;
 }
}

  • library-e exports YourLibrary like this:

    import { MyClass } from '@local/library-a';

export default class YourLibrary {
 public static setMyClass(myClass: MyClass): void {
 }
}

  • application's package.json imports both libraries and also a library-b which indirectly depends on version 2.0 of library-a

     "dependencies": {
   "typescript": "^1.8.10",
   "@local/library-b": "1.0.0",
   "@local/library-d": "1.0.0",
   "@local/library-e": "1.0.0"
 }

  • As a result, the node_modules folder must unavoidably create two copies of library-a@1.0.0 (even after you run "npm dedupe" with npm version 3.0!):

    C:\NpmInstallRepro\application>tree /a
+---lib
+---node_modules
|   +---.bin
|   +---@local
|   |   +---library-a
|   |   |   \---lib
|   |   +---library-b
|   |   |   \---lib
|   |   +---library-d
|   |   |   +---lib
|   |   |   \---node_modules
|   |   |       \---@local
|   |   |           \---library-a
|   |   |               \---lib
|   |   \---library-e
|   |       +---lib
|   |       \---node_modules
|   |           \---@local
|   |               \---library-a
|   |                   \---lib
|   \---typescript
|       +---bin
|       \---lib
\---src

  • application tries to do this:

   import { MyLibrary } from '@local/library-d';
   import { YourLibrary } from '@local/library-e';

   YourLibrary.setMyClass(MyLibrary.getMyClass());

  • The error looks like this:

    src/index.ts(5,24): error TS2345: Argument of type 'MyClass' is not assignable to parameter of type 'MyClass'.
Types have separate declarations of a private property '_data'.

  • There are two physical files (containing equivalent definitions):
    CompilerBug\application\node_modules@local\library-d\node_modules@local\library-a\lib\MyLibrary.d.ts
    CompilerBug\application\node_modules@local\library-e\node_modules@local\library-a\lib\MyLibrary.d.ts

Running the Repro

Extract the attached CompilerBug.tar.gz on a Windows PC and follow the steps in Instructions.txt. Note that this repro requires you to install sinopia, which is a private NPM server where you can publish the test packages. (Also note that the batch files are very simple and intended to be run multiple times, so the first time they will report some errors when they try to delete output folders that don't exist yet.)

Expected behavior:

I believe the compiler should determine that the MyLibrary.d.ts files are equivalent and treat them as the same class. If we're going to use classes in our public API at all, it seems that this must be supported, otherwise developers will have to do awkward "as any" workarounds when they randomly happen to hit a certain edge case for "npm install." In some cases (e.g. a class trying to extend a base class from a separate package) there is no reasonable workaround.

One idea would be for the compiler to examine the containing package.json for each file path, and check whether the version numbers are exactly the same. If there is a concern about a package being locally modified, "npm install" injects other fields in package.json that the compiler could use to determine whether the package is pristine or not.

In the past, it was suggested that we might be able to solve this using tsc path mapping. I'm not sure that this is the right approach. In the above example, we are not trying to remap the "application" project; we actually need to remap its indirect dependency "node_modules/@local/library-e" to find MyClass.d.ts in the "node_modules/@local/library-d" directory. Configuring the application to let us micromanage the module resolution in this way feels wrong. Basically our build scripts would be completely circumventing the module resolution algorithm that the compiler is supposed to provide for us.

If you could provide a compiler option to completely suppress TS2345, that would be a reasonable (temporary) workaround for us. This error is detecting a fairly obscure situation that is less likely to occur than the above scenario.

@vladima @dzearing @cliffkoh

blakeembrey commented

I believe it's the same as this issue I logged before: #7755.

octogonz commented

Your issue #7755 seems to be asking to permanently disable TS2345. I think I agree with @RyanCavanaugh that this could be dangerous. In fact, I would go a little further to say that I'm uncomfortable with structural equivalence even when there are no private members. For example, just because BankBalance.add(number) and ArrayOfNumbers.add(number) have compatible type signatures, this does not imply that their contracts have compatible semantics. The purpose of interfaces is to define these semantics, whereas the purpose of classes is to implement them. Personally I don't see any software engineering benefit of classes-as-interfaces (although maybe this TypeScript feature is forced by limitations of the underlying JavaScript language).

By contrast, my bug here involves the exact same source file from the exact same package version having two different paths on disk (due to the simpletonian design of the NPM package manager). I consider it a bug because the TypeScript compiler is supposed to support NPM, but the type system does not work correctly in this situation.

blakeembrey commented

@pgonzal I wasn't asking to disable it, just for a solution to something unexpected as you have found, but I was in the exact same situation. It was an identical definition from an identical .d.ts file, just bundled differently to NPM @types (hence the definition was 100% structurally the same, the only difference here is that you have two files and I had to module declarations).

blakeembrey commented

Maybe you'd be more interested in #6496. It's also the same issue style of issue.

mhegazy commented

This is a duplicate of #6496.

There are 2 files on disk, that are identical/compatible but they are not the same. at run-time given this structure, there are 2 node modules loaded. so the compiler behavior here is accurate.

The type system in TS is structural, meaning that things that "look" compatible
TypeStrong/ts-loader#324 are considered compatible regardless where they came from.

private does not fit in that model; the expectation of a private property is that is accessed only by same exact class that created it. To enforce that, the compiler checks the identity of the declarations, and two declarations are not the same.

The compiler supports npm link as a feature, in this case npm link is not used.

#6496 tracks adding support for this scenario.

grofit commented

I get this exact issue where NPM ends up including multiple instances (one root, 1-N nested) and it complains it doesnt know which d.ts to use. To just get on and get stuff finished I had to move all dependencies to peerDependencies on the re-useable modules, and then duplicate those dependencies in each lib as devDependencies this at least enforces that there is only root level modules but it is horrible and requires me to basically include each dependency twice AND it means all consumers need to satisfy the child dependencies, so when I have moduleA -> moduleB -> moduleC -> appA the app is having to consume all the dependencies from the downstream.

Is there a better solution for this? I am not using npm link in any way here so its not exactly the same as the npm link issue that keeps getting mentioned.

  • 👍1