Add option to leave out definitions for common modules + namespace models #219
Conversation
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
751a801 to
9fab292
Compare
|
Rebased to match master. Please let me know if there are any desired changes and I'll be happy to accommodate. |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
9fab292 to
cb28c8a
Compare
|
@slnode ok to test |
| @@ -374,7 +380,9 @@ if (typeof module !== 'undefined' && typeof exports !== 'undefined' && | |||
| }, | |||
| }; | |||
| }]) | |||
|
|
|||
| <% } // end if (defineLoopBackAuthRequestInterceptor) %> | |||
| defineLoopBackResource: <%= defineLoopBackResource %> | |||
There was a problem hiding this comment.
Hmm, what is the purpose of this line?
|
Hi @keithajackson, thank you for the pull request. Could you please add tests to verify your changes? If I am reading the code correctly, when module
// ...
.config(['$httpProvider', function($httpProvider) {
$httpProvider.interceptors.push('LoopBackAuthRequestInterceptor');
}])
.factory('LoopBackAuthRequestInterceptor', ['$q', 'LoopBackAuth',
function($q, LoopBackAuth) {
// ...
}])
// ^-- missing semicolonI am proposing to get rid of fine-grained flags |
| var defineLoopBackAuthRequestInterceptor = true; | ||
| var defineLoopBackResource = true; | ||
|
|
||
| if (excludeCommonModules === "true") { |
There was a problem hiding this comment.
Why a string? At this abstraction level, we should expect input arguments in the correct type and let callers to figure out how to convert string input into boolean values.
if (excludeCommonModules) {
// ...
}
|
@keithajackson hello, what's the status of this pull request? Are you still keen to finish it? |
|
Hi @bajtos - my apologies for letting this go stale - I'm working on writing tests and catching up to master right now. I'm running into some difficulty getting the test system up and running and was wondering if you might be able to give me a suggestion of what to try next. I've been following the instructions in When I run The second terminal outputs: I went into the The terminal running the test server outputs Does this issue sound familiar? Is there a server or Selenium webdriver I need to be running on port 9876 in addition to the test-server on 3033? Thank you so much for your time and assistance! |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
cb28c8a to
538edd7
Compare
persimmons
changed the title
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
538edd7 to
f3afff4
Compare
|
Hey @keithajackson, I am sorry to hear about the difficulties with getting the tests environment set up. Before I dig deeper, I'd like to check whether |
|
Hi @bajtos! All right - I think I've figured out the issue, at least on my machine. My It seems connected to this thread related to Angular v1.5 breaking compatibility with Phantom v1.x: angular/angular.js#13794 I've run it in these environments and found the same issue:
I'm unblocked on my issue, so I'll continue working on tests - but I would recommend either updating |
persimmons
force-pushed
the
feature/optional-common-modules
branch
2 times, most recently
from
24dcbdc to
8d089fc
Compare
|
Hi @bajtos , I've added tests covering the new features, but few builds are failing:
Do you have any recommendations for how to address these issues? |
| @@ -50,10 +50,14 @@ module.exports = function generateServices(app, options) { | |||
| apiUrl: arguments[2], | |||
| }; | |||
| } | |||
| var oldOptions = options; | |||
There was a problem hiding this comment.
AFAICT, this is unused - please remove.
There was a problem hiding this comment.
Ah yes - removed!
|
Hey @keithajackson, sorry for the long delay. I reviewed your changes, they looks mostly good to me. Please address the comments I left above. As for failing CI builds, I am afraid our CI system is still not stable and still not visible to public. Just make sure |
|
Can one of the admins verify this patch? |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
7b4b562 to
2f54ee5
Compare
|
@bajtos All comments adressed and code rebased against master - thanks so much for close review! Let me know if you have any other comments you'd like addressed or run into any surprises. I double-checked test after the update+rebase and it all looks good on my machine. |
|
Thank you for the quick update, @keithajackson! I run --- apidocs/loopback-core-master.js 2016-07-28 14:43:03.000000000 +0200
+++ apidocs/loopback-core.js 2016-07-28 14:43:18.000000000 +0200
@@ -6993,6 +6993,7 @@
}]);
+
module
.factory('LoopBackAuth', function() {
var props = ['accessTokenId', 'currentUserId', 'rememberMe'];What's your opinion? Do you feel like fixing this small detail, or should we ignore it? Other than that, the patch LGTM. Could you please squash all your commits into a single one and provide a descriptive commit message following the 50/72 rule? |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
2f54ee5 to
433e46f
Compare
|
@bajtos Fixed! I was able to find the blank line causing the documentation inconsistency and removed it. I've also squashed commits to match the commit style. Thanks again for your time and thoughtful comments. Let me know if there is anything else I can improve or help with! |
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
433e46f to
ee43271
Compare
When connecting to several LoopBack APIs from an Angular app, multiple definitions of LoopBackAuth and LoopBackResource will 'step on' each other, resulting in unpredictable behavior. In addition, if two APIs have models with the same names, these definitions will override each other, depending upon whichever API loaded last. This patch keeps the default behavior consistent, but provides an option to omit the module block that generates the LoopBackAuth, LoopBackAuthRequestInterceptor, and LoopBackResource components. It also provides an option to namespace models with the module name.
persimmons
force-pushed
the
feature/optional-common-modules
branch
from
ee43271 to
1c7ec4c
Compare
|
@keithajackson awesome! I have landed the patch, thank you for this great contribution 🎉 👏 |
|
Released as |
When connecting to several LoopBack APIs from an Angular app, multiple definitions of LoopBackAuth and LoopBackResource will 'step on' each other, resulting in unpredictable behavior.
This change keeps the default behavior consistent, but will omit the module block that generates the LoopBackAuth, LoopBackAuthRequestInterceptor, and LoopBackResource components if a fourth parameter (excludeCommonModules) is set to 'true' when generateServices is called.
This will enable loopback-sdk-angular to more seamlessly be used with multiple-API architectures, without having to manually delete sections of code generated by the SDK.