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

Fix module packaging #24

Merged

Conversation

devoto13
Copy link
Collaborator

@devoto13 devoto13 commented Mar 28, 2018

Well, packaging of Angular library is not straight-forward >.<

Finally got it working with both JiT and AoT. Check commits for more detailed explanations.

And it definitely worths to add integration tests to prevent regressions in the future.

It looks like Angular metadata resolver does not process them properly
and resolves symbols to undefined if they are coming from re-export. I
don't completely understand how it works, but after debugging this I
also checked https://github.com/angular/angular/tree/master/packages and
https://github.com/ng-bootstrap/ng-bootstrap/tree/master/src and none
seem to have index.ts files besides the main one.
If @component is present compiler attempts to handle this class as a real
component and fails because it was not added to the NgModule. What is
really needed is to force metadata collection on this class and it can
be achieved by putting some useless Angular decorator.

angular/angular#16566 (comment)
Copy link
Member

@robmadole robmadole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goodness gracious 😩

@devoto13
Copy link
Collaborator Author

devoto13 commented Mar 28, 2018

Oh, yeah! 😄

I have also checked the tree-shaking with imports from main. Well, they actually "just work". The only problem is that they work about 4-5 times slower on production builds (~20s to 80-90s). I think we can solve this by using sideEffects free modules from Webpack 4, so Webpack will do the heavy lifting instead of Uglify, but I'll need to look deeper into this topic. Maybe over the weekend :)

@mlwilkerson
Copy link
Member

Wow, thanks for untangling these knots, @devoto13.

@mlwilkerson mlwilkerson merged commit 332dd59 into FortAwesome:fix-module-packaging Mar 28, 2018
@robmadole
Copy link
Member

Thank you so much @devoto13!

mlwilkerson pushed a commit that referenced this pull request Mar 28, 2018
* Removed index.ts files from library

It looks like Angular metadata resolver does not process them properly
and resolves symbols to undefined if they are coming from re-export. I
don't completely understand how it works, but after debugging this I
also checked https://github.com/angular/angular/tree/master/packages and
https://github.com/ng-bootstrap/ng-bootstrap/tree/master/src and none
seem to have index.ts files besides the main one.

* Replaced @component annotation with @Injectable() on the abstract class

If @component is present compiler attempts to handle this class as a real
component and fails because it was not added to the NgModule. What is
really needed is to force metadata collection on this class and it can
be achieved by putting some useless Angular decorator.

angular/angular#16566 (comment)
@devoto13 devoto13 deleted the fix-module-packaging branch April 4, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants