Skip to content

Commit

Permalink
fix(ng1): grab injector from app. #451
Browse files Browse the repository at this point in the history
  • Loading branch information
@mlynch
mlynch committed Sep 1, 2016
1 parent 2f706de commit 2dc68a4
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/plugins/plugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ function callCordovaPlugin(pluginObj: any, methodName: string, args: any[], opts

function getPromise(cb) {
if (window.angular) {
let $q = window.angular.injector(['ng']).get('$q');
let $q = window.angular.element(document.body).injector().get('$q');
return $q((resolve, reject) => {
cb(resolve, reject);
});
Expand Down

13 comments on commit 2dc68a4

@mattlewis92
Copy link
Contributor

Choose a reason for hiding this comment

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

@mlynch I don't think this will ever work, see: http://codepen.io/anon/pen/QKWOPK?editors=1011

@mattlewis92
Copy link
Contributor

Choose a reason for hiding this comment

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

I've added a test suite here: #523 with the improved (not leaking $q onto the IonicNative) ng1 promises fix I proposed in the other issue, you're welcome to check out the branch and hack about to try and find a better solution, currently using the run block is the only way I can find though 😞 (I had a look to see how the ionic-cloud client works and that uses the run block too: https://github.com/driftyco/ionic-cloud/blob/master/src/angular.js#L54)

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016

Choose a reason for hiding this comment

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

hmm, works fine for me on angular 1.5.3, I wonder what the discrepancy is about.

screenshot 2016-09-02 11 08 26

@mattlewis92
Copy link
Contributor

@mattlewis92 mattlewis92 commented on 2dc68a4 Sep 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

That's really weird, it's not working in this codepen (check the console): http://codepen.io/anon/pen/QKWOPK?editors=1011 or in unit tests :/

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016

Choose a reason for hiding this comment

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

Okay I see, it starts to work once you're inside of a controller. Let me re-evaluate. I still think there's a cleaner way to do this

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016

Choose a reason for hiding this comment

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

More generally: it works after a $timeout, doesn't have anything to do with controller

@mattlewis92
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird how it only works after a timeout :/ Better to use document.querySelector('[ng-app]') than document.body in case the ng-app isn't on the body tag, other than that I think it does the trick! I'll update my PR with it now 😄

angular.module('myApp', [])
  .config(function($compileProvider) {
    //Disable runtime debug information in the compiler
    $compileProvider.debugInfoEnabled(false);
  })
  .run(function($q, $timeout) {
    $timeout(() => {
      console.log($q === angular.element(document.querySelector('[ng-app]')).injector().get('$q'));
    });
});

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What about injecting $rootElement and using it like this:

$rootElement.injector().get('$q')

That should work and is cleaner. Thoughts?

@mattlewis92
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you get the $rootElement from outside of angular though?

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016

Choose a reason for hiding this comment

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

oh, good call :D

So, the issue with [ng-app] is that it won't work if the user bootstrapped manually. I believe (and my tests have shown this), that using document.body will work even if they bootstrapped with the <html> node

@mattlewis92
Copy link
Contributor

Choose a reason for hiding this comment

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

document.body will work if ng-app is on the html or body tag, if it's somewhere inside the body tag it doesn't though. Same goes for if they bootstrap the app on an element that isn't the body or html tag :/

@mlynch
Copy link
Collaborator Author

@mlynch mlynch commented on 2dc68a4 Sep 2, 2016

Choose a reason for hiding this comment

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

hmm, okay. This might be a case where we have to just rely on ng-app. I'm fine with that for now.

@mattlewis92
Copy link
Contributor

@mattlewis92 mattlewis92 commented on 2dc68a4 Sep 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

PR sent #524 with the changes we discussed, I added a console warning as well for the edge cases where this might not work, so at least the user will be aware 😄

Please sign in to comment.