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

UpgradeAdapter should have a singleton method or something like this #8279

Closed
yjaaidi opened this issue Apr 27, 2016 · 10 comments
Closed

UpgradeAdapter should have a singleton method or something like this #8279

yjaaidi opened this issue Apr 27, 2016 · 10 comments

Comments

@yjaaidi
Copy link
Contributor

@yjaaidi yjaaidi commented Apr 27, 2016

I've been struggling a while with different kind of errors trying to make an hybrid AngularJS & Angular 2 app using UpgradeAdapter.downgradeNg2Component and UpgradeAdapter.upgradeNg1Component.
I couldn't understand why simple examples would work but not my app.
Well, simply because my app is splitted into multiple files and each file was creating a new instance of UpgradeAdapter while you should only have one singleton for the whole app as upgraded/downgraded components are stored inside this instance.

1 - Documentation, examples and boilerplates should show there should be only one single UpgradeAdapter instance per app.
2 - Maybe UpgradeAdapter should have a singleton method and all documentations and examples should use it.
3 - Making the constructor private could be interesting to avoid errors but it will break all current implementations and it will prevent us from having multiple apps on the same page.

As you can see in this example, using different instances causes an error.
http://plnkr.co/edit/ssWDBnR8DOpnQFzhneBL?p=preview

@yjaaidi
Copy link
Contributor Author

@yjaaidi yjaaidi commented Apr 27, 2016

Here is an error example when UpgradeAdapter can't find the upgraded component as the component was upgraded in another instance.

InstantiationError {_wrapperMessage: "DI Exception", _originalException: TypeError: Cannot read property 'scope' of null
    at new UpgradeNg1ComponentAdapter (http://localh…, _originalStack: "TypeError: Cannot read property 'scope' of null↵  …actory_LzJobList0 (viewFactory_LzJobList:1358:23)", _context: null, _wrapperStack: "Error: DI Exception↵    at InstantiationError.Wrap…   at viewFactory_LzApp0 (viewFactory_LzApp:74:1)"…}_context: null_originalException: TypeError: Cannot read property 'scope' of null
    at new UpgradeNg1ComponentAdapter (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:50292:53)
    at new UpgradeNg1ComponentAdapterBuilder.type.core_1.Directive.Class.constructor (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:50146:33)
    at http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:30128:52
    at Injector._instantiate (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29650:28)
    at Injector._instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29585:26)
    at Injector._new (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29574:22)
    at InjectorInlineStrategy.instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29074:31)
    at ElementDirectiveInlineStrategy.init (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12681:25)
    at new AppElement (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12358:29)
    at viewFactory_LzJobList0 (viewFactory_LzJobList:1358:23)_originalStack: "TypeError: Cannot read property 'scope' of null↵    at new UpgradeNg1ComponentAdapter (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:50292:53)↵    at new UpgradeNg1ComponentAdapterBuilder.type.core_1.Directive.Class.constructor (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:50146:33)↵    at http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:30128:52↵    at Injector._instantiate (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29650:28)↵    at Injector._instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29585:26)↵    at Injector._new (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29574:22)↵    at InjectorInlineStrategy.instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29074:31)↵    at ElementDirectiveInlineStrategy.init (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12681:25)↵    at new AppElement (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12358:29)↵    at viewFactory_LzJobList0 (viewFactory_LzJobList:1358:23)"_wrapperMessage: "DI Exception"_wrapperStack: "Error: DI Exception↵    at InstantiationError.WrappedException [as constructor] (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:5018:32)↵    at new InstantiationError (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:19127:17)↵    at Injector._instantiate (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29711:20)↵    at Injector._instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29585:26)↵    at Injector._new (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29574:22)↵    at InjectorInlineStrategy.instantiateProvider (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:29074:31)↵    at ElementDirectiveInlineStrategy.init (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12681:25)↵    at new AppElement (http://localhost:8000/assets/scripts/vendor.68938b50be4a110f1726.bundle.js:12358:29)↵    at viewFactory_LzJobList0 (viewFactory_LzJobList:1358:23)↵    at viewFactory_LzApp0 (viewFactory_LzApp:74:1)"causeKey: (...)context: (...)injectors: Array[1]keys: Array[1]message: (...)originalException: (...)originalStack: (...)wrapperMessage: (...)wrapperStack: (...)__proto__: WrappedException "<lz-app id="NG2_UPGRADE_0_lzApp_c0">"
@jeffwhelpley
Copy link

@jeffwhelpley jeffwhelpley commented Apr 27, 2016

@yjaaidi are you by chance going to be at ng-conf? I am not using the upgrade adapter myself, but @joeeames has some really good ideas on this topic and @PascalPrecht is going to run a workshop on it. I think they may have part of that workshop livestreamed if you are not going to ng-conf and they probably will publish a lot of their materials.

@yjaaidi
Copy link
Contributor Author

@yjaaidi yjaaidi commented Apr 27, 2016

Hi @jeffwhelpley thanks for your quick response. I would love to join you at the ng-conf but I'm in the french riviera which makes it a bit difficult to join you :)
Great! Well, I'll wait for any materials from @joeeames and @PascalPrecht.
I'm also interested in any discussions concerning ways of upgrading from ng1 to ng2 as I think that's it's a clue for ng2's success.

@joeeames
Copy link
Contributor

@joeeames joeeames commented Apr 27, 2016

@yjaaidi I would support making it have a singleton instance. Is there a use case for multiple instances? It's far too easy to create multiple instances which is wrong 99% of the time. This should be optimized for the majority use case, not the majority.

FYI yjaaidi, I will be publishing a course on upgrading on Pluralsight.com in the coming weeks.

@yjaaidi
Copy link
Contributor Author

@yjaaidi yjaaidi commented Apr 27, 2016

Right now, I can't imagine any multiple instances scenario as we are talking about upgrading angularjs 1 apps and it's not meant make multiple apps coexist on the same page.

+1 then for the singleton! (and documentation / examples update for any reader that has written upgrade examples).

Thanks Joe!

@joeeames
Copy link
Contributor

@joeeames joeeames commented Apr 27, 2016

I'm working with @teropa to update the official documentation as well on some other items, but we can make sure that the singleton issue is clearly communicated.

@yjaaidi
Copy link
Contributor Author

@yjaaidi yjaaidi commented Apr 27, 2016

Yeay!!!

As I'm an AngularJS teacher and coach, I'll get in touch with you guys (if you are interested ;)) with any issues I encounter with "beginners".

For example, naively calling "bootstrap" and "UpgradeAdapter.bootstrap" produces a not-that-human-readable-error like this one "No provider for $scope! (class0 -> $scope)".
Maybe, we should implement something that triggers an explicit error when calling both. Something like: "bootstrap and UpgradeAdapter.bootstrap can't both be called. Make a choice :p"

Should I open another issue and make a pull request for this?

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Dec 21, 2016

Is this still an issue for you @yjaaidi ?

@yjaaidi
Copy link
Contributor Author

@yjaaidi yjaaidi commented Dec 21, 2016

Hi @petebacondarwin,

Thanks for your message. I still didn't play with UpgradeModule but the new implementation should solve the issue as UpgradeAdapter is not used directly anymore.
Thus, the issue can be closed.

Younes

@yjaaidi yjaaidi closed this Dec 21, 2016
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Sep 10, 2019

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants