angular/angular

ngc emits JS for Typescript vendor files

filipesilva opened this issue · 9 comments

filipesilva commented

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

ngc will emit JavaScript files for vendor TypeScript files imported by the user app, even if these files are not part of the files included by tsconfig.

Expected behavior

Files that are not included in tsconfig should not be compiled.

Minimal reproduction of the problem with instructions

git clone https://github.com/filipesilva/bad-vendor-ngc
cd bad-vendor-ngc
npm i
npm run ngc

After the ngc compilation is finished, you can see non-factory JS files emitted for the angular2-click-outside library:

$ ls -lha out-tsc/app/node_modules/angular2-click-outside/
total 25K
drwxr-xr-x 1 kamik 197609    0 Nov  2 17:20 .
drwxr-xr-x 1 kamik 197609    0 Nov  2 17:20 ..
-rw-r--r-- 1 kamik 197609 1.3K Nov  2 17:20 clickOutside.directive.js
-rw-r--r-- 1 kamik 197609  761 Nov  2 17:20 clickOutside.directive.js.map
-rw-r--r-- 1 kamik 197609  782 Nov  2 17:20 clickOutside.directive.metadata.json
-rw-r--r-- 1 kamik 197609  197 Nov  2 17:20 clickOutside.directive.ngfactory.js.map
-rw-r--r-- 1 kamik 197609 1.5K Nov  2 17:20 clickOutside.directive.ngsummary.json

This library was published with TS files, and without metadata:

$ ls -lha node_modules/angular2-click-outside/
total 295K
drwxr-xr-x 1 kamik 197609    0 Nov  2 17:16 .
drwxr-xr-x 1 kamik 197609    0 Nov  2 17:16 ..
drwxr-xr-x 1 kamik 197609    0 Nov  2 17:16 .idea
-rw-r--r-- 1 kamik 197609   21 May 15  2016 .npmignore
-rw-r--r-- 1 kamik 197609    3 May 15  2016 .nvmrc
-rw-r--r-- 1 kamik 197609  282 Aug 19  2016 clickOutside.directive.d.ts
-rw-r--r-- 1 kamik 197609 2.8K Aug 19  2016 clickOutside.directive.js
-rw-r--r-- 1 kamik 197609 1.5K Aug 19  2016 clickOutside.directive.js.map
-rw-r--r-- 1 kamik 197609  682 Aug 19  2016 clickOutside.directive.ts
-rw-r--r-- 1 kamik 197609  601 May 15  2016 gulpfile.js
-rw-r--r-- 1 kamik 197609   66 Aug 19  2016 index.d.ts
-rw-r--r-- 1 kamik 197609  476 Aug 19  2016 index.js
-rw-r--r-- 1 kamik 197609  113 Aug 19  2016 index.js.map
-rw-r--r-- 1 kamik 197609   64 May 15  2016 index.ts
-rw-r--r-- 1 kamik 197609 1.5K May 15  2016 LICENSE
-rw-r--r-- 1 kamik 197609 1.7K Nov  2 17:16 package.json
-rw-r--r-- 1 kamik 197609  368 May 16  2016 README.md
-rw-r--r-- 1 kamik 197609  368 May 15  2016 tsconfig.json
-rw-r--r-- 1 kamik 197609   96 May 15  2016 typings.json

What is the motivation / use case for changing the behavior?

This behaviour is problematic because it hides improperly packaged libraries, and may fail compilation due to the app TS version being different from the TS version needed by the library.

Environment


Angular version: 5.0.0


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX   6.11.1
- Platform:   Windows

Others:

Related to angular/angular-cli#8263, angular/angular-cli#8216.

/cc @chuckjaz

tbosch commented

Analysis:

  • ngc includes .ts files into the compilation, as ngc incorrectly tells TS that this file is not in node_modules (via ts.ResolvedModule.isExternalModule = false).

Proposed fix:

  • change CompilerHost.resolveModuleName to only set isExternalModule to false if the file is a this.isSourceFile and it is a .d.ts file. We can't remove this logic altogether as otherwise our generated files for files in node_modules would not be emitted by TypeScript as TypeScript thinks they are not sources, as the file the original file in node_modules is not.
  • this is the same fix as for #19757
  • 👍1
Delagen commented

Seems this issue broke my build? Version 5.1.1 works fine. But latest does not generate JS factories, but emit metadata and definitions for files in node_modules

AllNamesRTaken commented

I am running 5.2.0 and using ngx-boostrap and this seem related to the fact that there is no X.component.js and only X.component.ngfactory.js in the generated node_modules folder. But the factories references have been rewritten to local paths making it impossible to bundle (using rollup).

The issue is well described here.

Either we have to keep the js files or not rewrite imports for those js files.

  • 👍1
mlc-mlapis commented

@AllNamesRTaken ... what are you using for building the lib?

AllNamesRTaken commented

@mlc-mlapis I am using ngc.

node_modules\.bin\ngc -p src\tsconfig.json

EDIT

It does work if I add this to tsconfig.json which indicates that it is related.

"include": [
	"./main.ts",
	"../node_modules/ngx-bootstrap/**/*.component.js",
	"../node_modules/ngx-bootstrap/**/*.class.js"
],

This forces me to for each such library add a number of specific includes which is less than optimal.

  • 🎉1
mlc-mlapis commented

@AllNamesRTaken ... hmm, we are using ng-packagr for building our packages due to the specs of Angular Package Format 5.0. Later when compiling an app ... those packages are already installed in node_packages as all others packages. And ... there are *.js versions of that packages already. NGC is able then to compile those packages to AOT mode ... so *.ngfactory.js are created. And finally ... we create bundles ... *.js and *.ngfactory.js are included to (+ we eliminate decorators because we don't need them in AOT mode).

With the above steps we don't need to add anything to includesection of tsconfig.json file.

  • 👍2
  • 🎉1
AllNamesRTaken commented

@mlc-mlapis But that does sound like you have a workaround. My point is that a clean ngc should not generate broken files.

Either it should not mangle the path, or it should add those js files that are imported using es2015 module format. That you can get it to work I am sure, but it feels like the law of least surprise is not in action here :).

alxhub commented

This has since been fixed - tested with Angular v10 RC.0.

angular-automatic-lock-bot commented

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.