AOT compiler doesn't generate *.ngfactory.ts files for 3rd party components in node_modules #11889

Closed
mattlewis92 opened this Issue Sep 24, 2016 · 20 comments

Comments

@mattlewis92

mattlewis92 commented Sep 24, 2016

I'm submitting a ... (check one with "x")

[x] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

When running ngc the compiler doesn't codegen components inside node_modules when the component isn't exported (export * from './my.component';) from the module. There are no errors emitted and all the *.metadata.json files for the libraries exist.

Expected behavior

For ngc to successfully codegen all 3rd party components without having to export the components publicly

Reproduction of the problem
Checkout this repo: https://github.com/mattlewis92/angular2-tv-tracker

npm install
npm run ngc

You will then see that only one ngfactory.ts module file is emitted for each 3rd party module inside of the aot/module_modules folder, but none of the other components are codegen'd. As you can see from the screenshot, the reference to the './directives/calendarTooltip.directive.ngfactory' file is created but the file itself doesn't exist.

screen shot 2016-09-24 at 15 35 24

If you need any more info let me know!

Please tell us about your environment:

OSX

  • Angular version: 2.0.X

    2.0.1

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

    N/A

  • Language: [all | TypeScript X.X | ES6/7 | ES5]
    Typescript

  • Node (for AoT issues): node --version =
    v5.11.0

@kylecordes

This comment has been minimized.

Show comment
Hide comment
@kylecordes

kylecordes Sep 24, 2016

I'm not deep enough to comment on whether this is a bug per se, but my impression from various words it is slowed by from the Angular team is that "For ngc to successfully codegen all 3rd party components" is not necessarily the actual expected behavior. Rather, once all the various bits are lined up correctly (possibly the case already?) it would be more expected that third-party components would ship with their ngc output already in place, and that ngc in a particular project would only do the work for that project, not for third-party stuff. Of course, disregard this once someone in the know answers for real.

I'm not deep enough to comment on whether this is a bug per se, but my impression from various words it is slowed by from the Angular team is that "For ngc to successfully codegen all 3rd party components" is not necessarily the actual expected behavior. Rather, once all the various bits are lined up correctly (possibly the case already?) it would be more expected that third-party components would ship with their ngc output already in place, and that ngc in a particular project would only do the work for that project, not for third-party stuff. Of course, disregard this once someone in the know answers for real.

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Sep 24, 2016

That was my first thought as well, but looking at the way that ng-bootstrap and angular material do it, shipping the *.metadata.json files with the module is sufficient.

On 24 Sep 2016, at 15:50, Kyle Cordes notifications@github.com wrote:

I'm not deep enough to comment on whether this is a bug per se, but my impression from various words it is slowed by from the Angular team is that "For ngc to successfully codegen all 3rd party components" is not necessarily the actual expected behavior. Rather, once all the various bits are lined up correctly (possibly the case already?) it would be more expected that third-party components would ship with their ngc output already in place, and that ngc in a particular project would only do the work for that project, not for third-party stuff. Of course, disregard this once someone in the know answers for real.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

That was my first thought as well, but looking at the way that ng-bootstrap and angular material do it, shipping the *.metadata.json files with the module is sufficient.

On 24 Sep 2016, at 15:50, Kyle Cordes notifications@github.com wrote:

I'm not deep enough to comment on whether this is a bug per se, but my impression from various words it is slowed by from the Angular team is that "For ngc to successfully codegen all 3rd party components" is not necessarily the actual expected behavior. Rather, once all the various bits are lined up correctly (possibly the case already?) it would be more expected that third-party components would ship with their ngc output already in place, and that ngc in a particular project would only do the work for that project, not for third-party stuff. Of course, disregard this once someone in the know answers for real.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 24, 2016

Member

@mattlewis92 yep only the metadata files are required. All the factory are generated by ngc when building the app so that any improvement made to the angular compiler over time could benefit to the libs.

Member

vicb commented Sep 24, 2016

@mattlewis92 yep only the metadata files are required. All the factory are generated by ngc when building the app so that any improvement made to the angular compiler over time could benefit to the libs.

@vicb vicb closed this Sep 24, 2016

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Sep 24, 2016

@vicb sorry I think you misunderstood me, I'm already shipping the metadata files with my libraries, but ngc isn't codegenning the 3rd party libraries properly. If you check out the repo I linked to you'll see what I mean 😀

@vicb sorry I think you misunderstood me, I'm already shipping the metadata files with my libraries, but ngc isn't codegenning the 3rd party libraries properly. If you check out the repo I linked to you'll see what I mean 😀

@vicb vicb reopened this Sep 24, 2016

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Sep 24, 2016

@vicb I did some digging and it turns out the cause of the issue was that the component classes weren't exported (export * from './name.component';) from the dependency in node_modules, only the NgModule class was exported.

So my question now is:

  • If this isn't intentional can it be fixed (My use case is I have some components in my libraries that are only used internally and I don't want people using them until their API's are stable - I currently leverage NgModule to include them in declarations but not in exports. If I now have to export them then people will start using these private components and will moan when the API changes).
  • If this is intentional / can't be fixed, then there should at least be an error message from ngc when the compiler couldn't codegen the component as it wasn't exported. I guess it should be able to detect this from the NgModule?

@vicb I did some digging and it turns out the cause of the issue was that the component classes weren't exported (export * from './name.component';) from the dependency in node_modules, only the NgModule class was exported.

So my question now is:

  • If this isn't intentional can it be fixed (My use case is I have some components in my libraries that are only used internally and I don't want people using them until their API's are stable - I currently leverage NgModule to include them in declarations but not in exports. If I now have to export them then people will start using these private components and will moan when the API changes).
  • If this is intentional / can't be fixed, then there should at least be an error message from ngc when the compiler couldn't codegen the component as it wasn't exported. I guess it should be able to detect this from the NgModule?
@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 24, 2016

Member

Thanks for reporting.

Yep, they should be exported.

Could you please attach a minimal repro. We'll work on a better error message. Thanks.

Member

vicb commented Sep 24, 2016

Thanks for reporting.

Yep, they should be exported.

Could you please attach a minimal repro. We'll work on a better error message. Thanks.

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Sep 24, 2016

Will get that done tomorrow, thanks!

Will get that done tomorrow, thanks!

@vicb vicb self-assigned this Sep 24, 2016

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Sep 25, 2016

@vicb I've put together a minimal repro here: https://github.com/mattlewis92/angular2-compiler-bug

There's an explanation in the readme, if you need more detail let me know 😄 (basically running npm run ngc from this repo should throw an error that node_modules/mwl-example-ng2-module/dist/src/demo.component.ts could not be codegen'd as it wasn't exported from the module - currently everything appears to compile with no errors)

@vicb I've put together a minimal repro here: https://github.com/mattlewis92/angular2-compiler-bug

There's an explanation in the readme, if you need more detail let me know 😄 (basically running npm run ngc from this repo should throw an error that node_modules/mwl-example-ng2-module/dist/src/demo.component.ts could not be codegen'd as it wasn't exported from the module - currently everything appears to compile with no errors)

@ammar91 ammar91 referenced this issue in primefaces/primeng Sep 26, 2016

Closed

AOT support #871

@vicb vicb assigned chuckjaz and unassigned vicb Sep 27, 2016

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 27, 2016

Member

@chuckjaz can you look at improving the error message ? Thanks

Member

vicb commented Sep 27, 2016

@chuckjaz can you look at improving the error message ? Thanks

@chuckjaz

This comment has been minimized.

Show comment
Hide comment
@chuckjaz

chuckjaz Sep 28, 2016

Member

@tbosch Can you look at this? From my initial investigation, this shouldn't be an error, we are just not emitting the code when we should.

The component is exported from its module and the only difference between the working and non-working versions is whether the component is exported from the module index which shouldn't make a difference.

Member

chuckjaz commented Sep 28, 2016

@tbosch Can you look at this? From my initial investigation, this shouldn't be an error, we are just not emitting the code when we should.

The component is exported from its module and the only difference between the working and non-working versions is whether the component is exported from the module index which shouldn't make a difference.

@jelbourn

This comment has been minimized.

Show comment
Hide comment
@jelbourn

jelbourn Sep 29, 2016

Member

@tbosch Having this problem with Material as well.

Member

jelbourn commented Sep 29, 2016

@tbosch Having this problem with Material as well.

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Sep 30, 2016

Member

The problem is as follows:

  • ngc compiles all files that the regular typescript compiler loads
  • if you look into calendar.module.d.ts, it does not reference any of the components any more, as TypeScript does not need their .d.ts file for type checking.

The solution is to change ngc to not just look into the files in the TypeScript program, but for all modules to look into its metadata and recursively also traverse referenced modules, directives, pipes, ...

Member

tbosch commented Sep 30, 2016

The problem is as follows:

  • ngc compiles all files that the regular typescript compiler loads
  • if you look into calendar.module.d.ts, it does not reference any of the components any more, as TypeScript does not need their .d.ts file for type checking.

The solution is to change ngc to not just look into the files in the TypeScript program, but for all modules to look into its metadata and recursively also traverse referenced modules, directives, pipes, ...

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Sep 30, 2016

Member

I am working on a fix...

Member

tbosch commented Sep 30, 2016

I am working on a fix...

@smith64fx

This comment has been minimized.

Show comment
Hide comment

+1

@tbosch

This comment has been minimized.

Show comment
Hide comment
@tbosch

tbosch Oct 21, 2016

Member

As we now also generate .ngfactory.ts files for directives, this bug becomes more severe.

Member

tbosch commented Oct 21, 2016

As we now also generate .ngfactory.ts files for directives, this bug becomes more severe.

vicb added a commit to vicb/angular that referenced this issue Oct 22, 2016

vicb added a commit to vicb/angular that referenced this issue Oct 24, 2016

vicb added a commit to vicb/angular that referenced this issue Oct 24, 2016

vicb added a commit to vicb/angular that referenced this issue Oct 24, 2016

vicb added a commit to vicb/angular that referenced this issue Oct 24, 2016

vicb added a commit to vicb/angular that referenced this issue Oct 24, 2016

@IgorMinar IgorMinar closed this in #12453 Oct 24, 2016

IgorMinar added a commit that referenced this issue Oct 24, 2016

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Oct 24, 2016

Awesome, thanks a bunch @vicb for fixing this! 😀

Awesome, thanks a bunch @vicb for fixing this! 😀

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 24, 2016

Member

That was actually a team work together with @tbosch and @chuckjaz

Have you tested the fix on your app ?

Member

vicb commented Oct 24, 2016

That was actually a team work together with @tbosch and @chuckjaz

Have you tested the fix on your app ?

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Oct 24, 2016

Ah, my thanks to both of them as well! :)

I just tested it and it doesn't actually seem to be working, it's failing with the error:

Error: Unexpected value 'DemoModule' imported by the module 'AppModule'
    at /Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:14065:37
    at Array.forEach (native)
    at CompileMetadataResolver.getNgModuleMetadata (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:14050:46)
    at /Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12819:42
    at Array.forEach (native)
    at analyzeNgModules (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12818:17)
    at OfflineCompiler.compileModules (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12876:20)
    at CodeGenerator.codegen (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler-cli/src/codegen.js:109:30)
    at codegen (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler-cli/src/main.js:7:81)
    at Object.main (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/tsc-wrapped/src/main.js:30:16)
Compilation failed

Minimal repro here: https://github.com/mattlewis92/angular2-compiler-bug

Ah, my thanks to both of them as well! :)

I just tested it and it doesn't actually seem to be working, it's failing with the error:

Error: Unexpected value 'DemoModule' imported by the module 'AppModule'
    at /Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:14065:37
    at Array.forEach (native)
    at CompileMetadataResolver.getNgModuleMetadata (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:14050:46)
    at /Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12819:42
    at Array.forEach (native)
    at analyzeNgModules (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12818:17)
    at OfflineCompiler.compileModules (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler/bundles/compiler.umd.js:12876:20)
    at CodeGenerator.codegen (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler-cli/src/codegen.js:109:30)
    at codegen (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/compiler-cli/src/main.js:7:81)
    at Object.main (/Users/mattlewis/Code/open-source/angular2-compiler-bug/node_modules/@angular/tsc-wrapped/src/main.js:30:16)
Compilation failed

Minimal repro here: https://github.com/mattlewis92/angular2-compiler-bug

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Oct 24, 2016

Member

@mattlewis92 the issue is actually in your code !

We just went into a debug session with @tbosch, you problem is in your nested module,
its package.json should be

  "peerDependencies": {
    "@angular/core": "angular/core-builds",
    "rxjs": "^5.0.0-beta.12",
    "zone.js": "^0.6.25"
  },

while it was

  "dependencies": {
    "@angular/core": "^2.0.1",
    "rxjs": "^5.0.0-beta.12",
    "zone.js": "^0.6.25"
  },

What happens is that the module had its own version of angular/core and then annotations are not recognized.
This change would make the module use the annotations from the single angular install.

We'll improve UX by printing an error message in this case.

Thanks for reporting this issue.

Member

vicb commented Oct 24, 2016

@mattlewis92 the issue is actually in your code !

We just went into a debug session with @tbosch, you problem is in your nested module,
its package.json should be

  "peerDependencies": {
    "@angular/core": "angular/core-builds",
    "rxjs": "^5.0.0-beta.12",
    "zone.js": "^0.6.25"
  },

while it was

  "dependencies": {
    "@angular/core": "^2.0.1",
    "rxjs": "^5.0.0-beta.12",
    "zone.js": "^0.6.25"
  },

What happens is that the module had its own version of angular/core and then annotations are not recognized.
This change would make the module use the annotations from the single angular install.

We'll improve UX by printing an error message in this case.

Thanks for reporting this issue.

@mattlewis92

This comment has been minimized.

Show comment
Hide comment
@mattlewis92

mattlewis92 Oct 24, 2016

Ah, npm would have been deduping it before I was using the nightly builds from github. Thanks a bunch for looking into that and apologies for wasting your time. I've published a new version of the 3rd party lib and I can confirm everything is getting codegen'd properly now 👍

Ah, npm would have been deduping it before I was using the nightly builds from github. Thanks a bunch for looking into that and apologies for wasting your time. I've published a new version of the 3rd party lib and I can confirm everything is getting codegen'd properly now 👍

@tsvetomir tsvetomir referenced this issue in telerik/kendo-angular Nov 3, 2016

Closed

Responsive Charts #73

@mattlewis92 mattlewis92 referenced this issue in ionic-team/ionic-app-scripts Dec 21, 2016

Closed

Compile issue on 0.0.47 #550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment