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

Fixes decorators for AOT building #29

Merged
merged 2 commits into from
Dec 31, 2016
Merged

Fixes decorators for AOT building #29

merged 2 commits into from
Dec 31, 2016

Conversation

sconix
Copy link
Contributor

@sconix sconix commented Dec 30, 2016

I still got AOT compilation errors when using decorators after your last changes (using configure instead of forRoot) since the decorators contained lambda functions.

This PR fixes the decorators to work when using AOT. I also made the forRoot to work for AOT since I think using the forRoot is the "standard" way to configure modules. I still kept the configure function so this is backwards compatible, i.e. you can using configure + Ng2Webstorage module or just Ng2Webstorage.forRoot(). In addition this new way of handling configuration allows providing the configuration separately through normal provide mechanism.

Let me know if you want me to change something, I tried to follow your coding style etc.

@PillowPillow
Copy link
Owner

That seems really great. I'll looking that tonight. thanks a lot.

@PillowPillow PillowPillow self-requested a review December 30, 2016 20:53
@sconix
Copy link
Contributor Author

sconix commented Dec 30, 2016

Thanks. I am still learning about AOT so I am not 100% sure if the way I solved the decorator issue is the best, but at least this way it seems to work and the compiler is happy. I tested with latest angular-cli both normal build and with --aot parameter. Without the changes to the decorators the compiler gave the usual symbol / lambda function calls error when using the --aot parameter.

The configuration part is pretty similar to the setup I am using in my libraries and I have seen similar ways used in other Angular 2 libs as well. I just adapted it so that your existing configure call works as well if some people has already switched to use that. I had some trouble with that method in our app where we have separate configuration module that provides configuration for all the modules that need configuration (we do this to have all configuration in one place and only load the actual modules in the app modules we need them). Now with the provide mechanism it works really nicely with all these different ways (configure, providing configuration separately and using forRoot).

@PillowPillow
Copy link
Owner

I tried the current version with the last angular cli version and the aot parameter and i didn't encounter any error. Are you sure you tried to use the v1.4.3?
Anyway i like the forRoot refacto you did and all platforms seem to work with. I'll merge it to the next build.

@PillowPillow PillowPillow merged commit cf54d86 into PillowPillow:master Dec 31, 2016
@PillowPillow PillowPillow removed their request for review December 31, 2016 02:04
@sconix
Copy link
Contributor Author

sconix commented Dec 31, 2016

Yes I tried to use with 1.4.3 and the error was there even when I pulled your master and made my own build, but it might be because we are using newer versions for some packages that the projects made with latest angular-cli. So I suspect that there might be a version combination that does not complain about the 1.4.3, but at least this implementation works in our project and newly created angular-cli project.

Thanks for merging this! This library was the last piece in our rather big project to get it build with AOT. Still have to figure out why our AOT build is bigger than JIT, but that is hopefully an angular-cli bug :)

@PillowPillow
Copy link
Owner

Maybe i have an answer for you. angular-cli#2799
The last angular cli version fixed an issue with the decorators and AoT but the generated files are now larger.

@sconix
Copy link
Contributor Author

sconix commented Dec 31, 2016

I noticed a similar issue so it seems that angular-cli still needs some work. So will have to wait a bit to see the full benefits of the aot.

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.

2 participants