configurable directory support for component and middleware#171
configurable directory support for component and middleware#171ashwinik001 wants to merge 8 commits intostrongloop:masterfrom ashwinik001:component-and-middleware-rootpath-configuration
Conversation
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
|
@slnode ok to test |
|
Hi @dopeddude, thank you for the pull request. Could you please add unit-tests to verify your new change and prevent regressions in the future? |
|
@bajtos : sure, I will. Thanks. I will update my PR to include the unit-tests. |
…on custom directory support
|
Hi @bajtos , I have updated my code to include some unit-tests. Please review and let me know your thoughts on it. Regards. |
|
@dopeddude thank you for the update, I'll take a look. Could you please sign our CLA in the mean time? I cannot land the patch without that. https://cla.strongloop.com/agreements/strongloop/loopback-boot |
|
@bajtos : I have signed the CLA. |
test/compiler.test.js
Outdated
| it('Suports `middlewareRootDir` Option And ' + | ||
| 'emits middleware instructions', function() { | ||
| testMiddlewareRegistration('loopback/server/middleware/url-not-found', | ||
| sourceFileForUrlNotFound, true); |
There was a problem hiding this comment.
I find it rather difficult to understand what's happening under the hood and how is this different from the test above, because the single true argument does not convey almost any information.
I believe one test is enough to verify that middleware registration looks at the correct json file, there is no need to test all different flavours of middleware name, thus testMiddlewareRegistration can remain unmodified.
it('supports `middlewareRootDir` option', function() {
var middlewareJson = {
custom: {
'loopback#urlNotFound': {}
}
};
var customDir = path.resolve(appdir.PATH, 'custom');
fs.mkdirsSync(customDir);
fs.writeJsonSync(path.resolve(customDir, 'middleware.json'), middlewareJson);
var instructions = boot.compile({
appRootDir: appdir.PATH,
middlewareRootDir: path.resolve(appdir.PATH, 'custom')
});
expect(instructions.middleware).to.eql([{
sourceFile: sourceFileForUrlNotFound,
config: { phase: 'custom' }
}]);
);|
@dopeddude I sincerely apologise for the long delay here. If you are still keen to finish, then please simplify the new tests per my comments above. Let me know what do you think! |
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
…into component-and-middleware-rootpath-configuration
|
Hi, @bajtos, apologies for the delay. |
|
Landed via 1382e8a, thank you @dopeddude for the contribution and your patience with us. |
|
Released as |
Hi,
In one of my project because of increasing number of configuration files due to number of deployment environments, I needed to separate out the
app-config, datasources, middleware, component-configin separate folders. Hence, this PR.It was a simple change to start the support for making
component-configandmiddlewareroot directory configurable.Please review.
Thanks.