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

[RC.x] Injecting missing import throws Cannot read property query of null #8245

Closed
ericmartinezr opened this issue Apr 26, 2016 · 10 comments · Fixed by #9065
Closed

[RC.x] Injecting missing import throws Cannot read property query of null #8245

ericmartinezr opened this issue Apr 26, 2016 · 10 comments · Fixed by #9065

Comments

@ericmartinezr
Copy link
Contributor

ericmartinezr commented Apr 26, 2016

Hard to explain in the title

Steps to reproduce and a minimal demo of the problem

See http://plnkr.co/edit/BpjzGxQU5CEStzmWdWfh?p=preview , note that I correctly imported Renderer, but I forgot to add ElementRef. Renderer is undefined when it shouldn't.

What steps should we try in your demo to see the problem?

Run the plnkr and see the console

Current behavior

Now if I inject something I forgot to import no error is thrown and all of the classes injected in the constructor are undefined, even if they're correctly imported

Expected/desired behavior

It should work as <= b.15 throwing an error like

Cannot resolve all parameters for 'App'(Renderer, ?)

See plnkr http://plnkr.co/edit/aP6UWXsDOYaz3yV9U7Pp?p=preview with b.15

Other information

@ericmartinezr
Copy link
Contributor Author

ericmartinezr commented May 6, 2016

With RC.x an error is thrown but it's not really helpful

VM764 browser_adapter.js:77 EXCEPTION: TypeError: Cannot read property 'query' of null

http://plnkr.co/edit/fkeQoc562NIEWJh2WwMd?p=preview

Title updated to reflect the changes in RC.x

@ericmartinezr ericmartinezr changed the title [beta.16 regression] When injecting a non-imported class all of them are undefined [RC.x] Injecting missing import throws Cannot read property query of null May 6, 2016
@ericmartinezr
Copy link
Contributor Author

This is not really about missing imports. To be more exact is about injecting undefined items in the constructor, for a repro using custom classes. For a repro see the plnkr on this issue #8573

@pkozlowski-opensource
Copy link
Member

@ericmartinezr I'm trying to reproduce this under non-plunker conditions (ng2 test suite) and it is super-hard since if you compile things with TS you are going to get a very descriptive error: error TS2304: Cannot find name 'ElementRef'.

So I'm starting to suspect that this is more of a TS + SystemJS thing. I mean: it should have been reported by SystemJS.

I will try some more to reproduce this under non-plunker conditions but it would be interesting to check with you / anyone else if you've ever seen this error outside plunks / SystemJS?

@ericmartinezr
Copy link
Contributor Author

@pkozlowski-opensource you're absolutely right, I can't reproduce it outside a plnkr, compilers and IDE complain. That being said there's clearly a regression on the error message as stated in the original message, previous beta.16 the error message was clear

Cannot resolve all parameters for 'App'(Renderer, ?)

Now it's that query of null thing.

Should we assume then that we all must be using IDE + compiler? In that case we won't see this error message and angular could avoid checking against it.

@rlegrand
Copy link

rlegrand commented Jun 7, 2016

@pkozlowski-opensource
@ericmartinezr

Hi,
The one I added in issue 8506 you just closed comes from a real case (outside of plunker): http://plnkr.co/edit/2GQwDiDAm3S0dGWEAIFD?p=preview

I just used plunker to share the problem, issue is exactly the same with tsc.

@pkozlowski-opensource
Copy link
Member

Should we assume then that we all must be using IDE + compiler? In that case we won't see this error message and angular could avoid checking against it.

Nope. It just that it is not as severe as was initially thinking. Anyway, I've managed to reproduce it by declaring dependency on an interface (which are undefined after TS transpiles).

That being said there's clearly a regression on the error message as stated in the original message, previous beta.16 the error message was clear

Yes, it clearly is and this is why we need to fix it. On it.

@rlegrand
Copy link

If you take my plunk and replace rc.1 by rc.2 in config.js, you will see that the error message has changed "Can't resolve all parameters for ManageDefinitions" but is still there.
Does it mean the issue comes from system? I don't find the answer in this issue.

@ericmartinezr
Copy link
Contributor Author

@rlegrand you have a circular import ManageDefinitions imports MenuService and MenuService imports ManageDefinitions, that's causing your issue.

@rlegrand
Copy link

Ahhh ok!
Thank you

KiaraGrouwstra pushed a commit to KiaraGrouwstra/angular that referenced this issue Jun 21, 2016
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants