Skip to content
This repository was archived by the owner on Feb 9, 2020. It is now read-only.

Fix loader for decorator and component #284

Merged
merged 2 commits into from
Jan 11, 2016
Merged

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Jan 5, 2016

The changes seem very straightforward, since the custom loader is mainly copy / paste.
I've additionally included a change for better error messages.

Question is, how do I test if this works? Can I run a version of the plugin locally?

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

I guess you could load your local copy as unpacked extension and test it, but there should be automatic tests in place. I remember there was a discussion some time ago about setting up unit-/integration-testing against actual apps, but I'm not sure if this ever moved forward.

@Narretz
Copy link
Contributor Author

Narretz commented Jan 5, 2016

There seem to be some tests, but the build fails.
I think we should just test this manually, and merge it.

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

One problem with this change is that it might behave strangely if the used version of angular does not support decorator() or component(). I.e. if someone is (incorrectly) trying to use .decorator() or .component() with a version of angular that does not have them, they should get an error, but they won't if they test it with Batarang enabled.

I am not sure it is worth fixing it though...

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

I tried this and it throws an error because isFunction() is not defined.

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

Now, identifierForController is not defined.

@gkalpak
Copy link
Member

gkalpak commented Jan 5, 2016

This is getting kind of out of control. There must be a better way, than defining a custom loader in Batarang that is mostly/only copy-paste.

@btford or anyone else, any idea why Batarang needs its own loader.js in the first place ?

@Narretz
Copy link
Contributor Author

Narretz commented Jan 5, 2016

For compat in 1.2 and 1.3: 6a0ae94 and the mentioned issues

I've tried to recreate the original error to see if a different change is possible, but I haven't been able to reproduce it.

@Narretz
Copy link
Contributor Author

Narretz commented Jan 5, 2016

I have tested with various configurations, and the easiest way to provoke the original error that the loader is supposed to fix is to run angular with version 1.2.29, but use the the angular-loader file from >=1.3.0

So do npm install angular-loader
And in hint.js change this line:

// require('./loader.js');
require('angular-loader');

Then the app should fail with a provider not found error.
Essentially, the patch in the custom loader changes the behavior of loading the config block to match that of angular 1.3. The major work for this change in 1.3 was in the $injector, so it's indeed a strange workaround, which basically adds the BC into 1.2.

Regarding identifierForController, I assume that is in the angular closure, so we have no access to it if the loader is used separately. This should also be an issue if people use the loader for async module loading.

@gkalpak
Copy link
Member

gkalpak commented Jan 8, 2016

After reading angular/angular-hint#39, I'm still not sure why Batarang needs a custom loader. If people use matching versions of loader and angular, shouldn't things just work fine ?

@petebacondarwin
Copy link

Now that we have refactored the loader in AngularJS 1.5.0 - we need to change this PR accordingly. See angular/angular.js#13692

@Narretz
Copy link
Contributor Author

Narretz commented Jan 11, 2016

I've updated the PR.
@gkalpak Before there was a custom loader, batarang would require the default angular-loader, so that ngHint can hook into it before the module loader gets set up. So basically your app will use the loader from batarang.

@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

LGTM

@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

It's been a while since travis was last happy with batarang. Is it because of the old Chrome version ?

@SomeKittens
Copy link
Member

Fantastic, thanks so much!

@gkalpak not sure, we have an issue open but I haven't looked into it much, just ran the tests manually.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants