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

allowUnreachableCode has to be turned off due to third party libraries. #10233

Closed
Vaccano opened this issue Aug 9, 2016 · 9 comments
Closed
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@Vaccano
Copy link

Vaccano commented Aug 9, 2016

This is a feature request:

I get the following error when I build my webpack project:

[default] C:\myPath\my-project\node_modules\bluebird\js\release\util.js:201:5
Unreachable code detected.
[default] Checking finished with 1 errors

This is a problem in the bluebird npm library.

But I don't want to have errors in my build. So I have to add this to my tsconfig.json to get this error to turn off:

"allowUnreachableCode": true

But that is kind of a shame, because I don't want to allow unreachable code in my code, I just don't want to see errors for unreachable code that is in my node_modules. (I can't change node_module files without messing up my autobuild.)

So:
I am seeing code in node_modules inspected for unreachable code.

What I would expect (or want) to see is that node_modules is excluded from the unreachable code inspection.

In case it matters, here is my tsconfig.json:

{
  "compilerOptions": {
    "target": "es2015",
    "module": "es2015",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "moduleResolution": "node",
    "sourceMap": true,
    "allowJs": true,
    "allowUnreachableCode": true
  },
  "awesomeTypescriptLoaderOptions": {
    "doTypeCheck": true,
    "forkChecker": true,
    "useBabel": true,
    "useCache": true,
    "babelOptions": {
      "presets": ["es2015-loose-native-modules"]
    }
  },
  "exclude": [
    "node_modules",
    "dist",
    "release",
    "index.js",
    "webpack.config.js",
    "config"
  ]
}
@RyanCavanaugh
Copy link
Member

node_modules being in your exclude list means we shouldn't see the JS by default. The fact that you do see it likely means you're importing it but there's no definition file. Do you have a .d.ts for bluebird somewhere, and can you confirm it's being picked up by the compiler?

@weswigham
Copy link
Member

Although bluebird does actually contain legitimately unreachable code:

function toFastProperties(obj) {
    /*jshint -W027,-W055,-W031*/
    function FakeConstructor() {}
    FakeConstructor.prototype = obj;
    var l = 8;
    while (l--) new FakeConstructor();
    ASSERT("%HasFastProperties", true, obj);
    return obj;
    // Prevent the function from being optimized through dead code elimination
    // or further optimizations. This code is never reached but even using eval
    // in unreachable code causes v8 to not optimize functions.
    eval(obj);
}

It just has a good reason for it.

@Vaccano
Copy link
Author

Vaccano commented Aug 9, 2016

I do have a .d.ts file for it.
It is located in typings/globals/bluebird/index.d.ts.

I know visual studio is picking it up because when I added the .d.ts file, this statement lost the red error squiggles:

import * as Bluebird from 'bluebird';
Bluebird.config({ warnings: false });

I assume the compiler is picking it up or it would fail compilation as well (because it would not know what Bluebird.config was).

@szahn
Copy link

szahn commented Aug 22, 2016

+1 Experiencing this inconvenience as well.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 22, 2016

If you are including the .js code from a different project, it is sort of your code at this point from the perspective of the compiler. the options are to include the .d.ts instead (.d.ts files have no statements, so no unreachable code by definition), or use a linter like tslint that allows you more fine grained control on what rules run on which files.

All the compiler flags/options are compilation wide knobs, I do not think we intend to enable any file-level control at the time being.

@mhegazy mhegazy added Suggestion An idea for TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed Out of Scope This idea sits outside of the TypeScript language design constraints Too Complex An issue which adding support for may be too complex for the value it adds labels Aug 22, 2016
@mhegazy mhegazy closed this as completed Aug 22, 2016
@Vaccano
Copy link
Author

Vaccano commented Aug 23, 2016

@mhegazy
Just to be clear, I am "including" the code by doing an NPM install of it.

If that counts as "including" the code then that is fine, but most every JavaScript project out there has at least one other library in it via a package manager. If that is not supported then it makes the unreachable code feature very hit and miss when allowJs is on. (It just comes down to luck if the packages have unreachable code or not.)

However, if that is the way it has to be because of Design Limitations and the fix being Too Complex then that is OK.

As a side note, I do have the d.ts file included as well (from Definitely Typed.)

@mhegazy
Copy link
Contributor

mhegazy commented Aug 24, 2016

I see the issue now. we have made a change in TS 2.0 to automatically load .js files from node_modules if --allowJS is specified. this brought the .js files into your project. I do not think this was the intent on your side, though i could be wrong.

as part of fixing #10460 we should be disabling this by default, so you only get it if you set --maxNodeModuleJsDepth 2 for instance. I think this would address the issue here since you already have the declaration file.

If i may ask, do you have .js files in your compilation other than the ones the compiler is automatically pulling them from node_modules?

@Vaccano
Copy link
Author

Vaccano commented Aug 25, 2016

I do have a .js file in my compilation.

Only one that has one line in it. (To declare a global variable because I could not get Typescript to do it. See this answer on SO if you want details http://stackoverflow.com/a/38906665/16241 )

@sandersn
Copy link
Member

#10538 may fix this as @mhegazy mentioned above, by reducing the default maxNodeModuleJsDepth to 0. Can you check with typescript@next in a day or two? (Travis CI seems to be down so I'm not sure when we'll get a nightly build.)

@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
Design Limitation Constraints of the existing architecture prevent this from being fixed Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests

6 participants