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(upgrade): ng1 compiler throws because ng2.Injector controller is missing #6655

Closed
0x-r4bbit opened this issue Jan 22, 2016 · 19 comments
Closed
Assignees
Labels
help wanted An issue that is suitable for a community contributor (based on its complexity/scope).

Comments

@0x-r4bbit
Copy link
Contributor

Stumbled upon something weird.

I have an Angular 1 app using the UI-Router. Which I started to upgrade using ngUpgrade. The default state configuration is as simple as:

...
.state('default', {
  abstract: true,
  template: `
    <cr-header></cr-header>
      <section class="thtrm-l-main">
        <div class="thtrm-l-constraint">
          <ui-view></ui-view>
        </div>
      </section>
  `
});

First component I wanted to upgrade was <cr-header>. There's nothing special about it. So I did it and downgraded it a la:

app.directive('crHeader', adapter.downgradeNg2Component(ClassroomHeader));

Running this in the browser throws the following error (while rendering works fine):

screen shot 2016-01-23 at 00 41 49

Angular 1 compiler basically complains because it can't find controller ng2.Injector as requested in crHeader directive.

It turns out that downgradeNg2Component() returns a directive that always requires an ng2.Injector controller as defined in: https://github.com/angular/angular/blob/master/modules/angular2/src/upgrade/upgrade_adapter.ts#L505

I was confused why this errors is thrown, because I'm not upgrading the first time and it never happened before to me.

I git blameed the file and that particular line of code and saw that it was introduced in 059e8faa, so this has been here since a long time, which brought me to the conclusion that there's something in my code base that causes the error.

After inspecting the error I saw that <ui-view class="ng-scope"> snippet in the stack trace. Even though I doubted that it could've anything to do with it, I remove <ui-view> from the template to see if this error still occurs. It doesn't

So apparently having a <ui-view> directive in a template where also a downgraded ng2 component is compiled, causes this error.

The stack trace shows updateView call in ui-router, which can be inspected here: https://github.com/angular-ui/ui-router/blob/2d23875c211bf3169fbec5c24e05a351b6b969dc/release/angular-ui-router.js#L3959

thoughtram-bot pushed a commit to thoughtram/classroom-app that referenced this issue Jan 22, 2016
This also introduces an error thrown when `crHeader` directive
is compiled by A1 and `<ui-view>` is used in the same template
as the directive.

Related issue: angular/angular#6655
@0x-r4bbit
Copy link
Contributor Author

If anyone wants to take a look at the commit that introduces the error, I've pushed the status to this branch here: thoughtram/classroom-app@9a7a538

@mhevery
Copy link
Contributor

mhevery commented Jan 22, 2016

Yes, ng2 requires parent ng2 Injector. The way we get it is by walking the DOM tree using the require property in DDO. We assume that some parent object has published it. Looks like in your case the parent never publishes it, hence the error. What is puzzling to me is why would adding / removing the <ui-view> cause this.

My guess would be that the ui-router is doing something which breaks this. If I had to guess, I would think that the ui-router compiles/links the DOM tree before it inserts it into the proper location in the DOM. The late linking causes the ngUpgrade to not find the injector.

Can you try this with dev code (not minified so that we have propert stack traces). That way we can see what the ui-router is doing which is breaking this.

@mahajansohan312
Copy link

Did you guys get a solution for this said problem? or there is a workaround for this?

@asqwrd
Copy link

asqwrd commented Feb 4, 2016

running into this issue as well

@mhevery mhevery self-assigned this Feb 7, 2016
@dogen-z
Copy link

dogen-z commented Feb 10, 2016

Waiting for this fix desperately , could be a deal breaker for upgrading our app to ng2, unless we have a work around .

@kvantetore
Copy link

@mhevery, is there a particular reason the ng2 injector is provided through require rather than a closure from UpgradeAdapter.bootstrap? It seems like a fragile solution as many components surely will compile before attaching to the main DOM tree?

AFAICT *ngIf is in some cases compiling before attaching to the main DOM tree, invoking the issue observed here.

I was working on a plunkr to demonstrate this, but got blocked by #7525.

@mhevery
Copy link
Contributor

mhevery commented Mar 11, 2016

The Injector in ng2 is hierarchical, which is why we walk the DOM to get it (At different points in the DOM there will be different ng2 injector.). Of the top of my head I don't see how this can be done in closure, but maybe you could give it a try?

@mhevery mhevery added this to the Angular 2 Release Candidate milestone Mar 11, 2016
@kvantetore
Copy link

That does make sense. However, it seems like "ng2.InjectorController" only gets set once, on the root element. I guess it should be set by the UpgradeNg1ComponentAdapter?

@yjaaidi
Copy link
Contributor

yjaaidi commented May 16, 2016

Hi!
I'm facing the same issue while trying to unit-test downgrader ng2 components:

http://embed.plnkr.co/fPD0zUTr2s1MCgn66dDS/

See console log on plunker.

@yjaaidi
Copy link
Contributor

yjaaidi commented May 16, 2016

Oups. Here's the right plunker link: https://embed.plnkr.co/n0KxZu/

@mhevery
Copy link
Contributor

mhevery commented May 16, 2016

Would anyone be willing to take on this issue and create a PR?

@mhevery mhevery added the help wanted An issue that is suitable for a community contributor (based on its complexity/scope). label May 16, 2016
@hannahhoward
Copy link
Contributor

I'll take a look.

@hannahhoward
Copy link
Contributor

PR submitted. See #8684.

@yjaaidi
Copy link
Contributor

yjaaidi commented May 17, 2016

YEAY! Thanks!

@hannahhoward
Copy link
Contributor

#8684 is merged. can anyone test if the original UI router issue is fixed? (I just wrote unit tests)

@hannahhoward
Copy link
Contributor

is this issue resolved? I submitted a PR a while ago that I believe is in RC2.

@IgorMinar
Copy link
Contributor

resolved

@yjaaidi
Copy link
Contributor

yjaaidi commented Sep 15, 2016

Hurray !

@angular-automatic-lock-bot
Copy link

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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted An issue that is suitable for a community contributor (based on its complexity/scope).
Projects
None yet
Development

No branches or pull requests

10 participants