Returning updated swaggerObject

[pktippa]

The pull request is fix for #140

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[pktippa]

@bajtos can you please accept this pull request?

[bajtos]

Hi @pktippa, thank you for the pull request. To be honest, I don't see how moving a variable declaration (not initialisation!) to a higher scope solves the problem.

Please add a unit-test that fails with the current implementation and passes with your fix in place - that's the best way how to show the correctness of the proposed solution. You can take a look at the unit-test in #22 for inspiration, just note that it has to be updated quite a lot to fit into the current codebase.

[bajtos]

@slnode ok to test

[bajtos]

@pktippa I am not sure how familiar you are with GitHub pull request. When reworking this patch, please keep using the same git branch & github pull request, do not open a new one. You can simply keep adding more commits here, as we can clean the history at the end, just before merging the code.

[pktippa]

@bajtos I am not that familiar with writing test cases, please help me.

[bajtos]

@pktippa Start with reading the unit-test in #22, it's a good one. Rework it to work in the current codebase. Then think about any edge cases that would not be covered by that test and write additional tests for these edge cases.

When adding a new test, it's important to see it fail for the reason you expected. This has multiple purposes: it ensures the test actually tests something (as opposed to tests that always passes because they are based on incorrect assumptions) and also that the failure message is helpful enough to identify the source of the problem.

[pktippa]

@bajtos The issue is with express itself. I have created a sample express app using express generator and added routes to create dynamic routes and respond with global and local scope varaibles, added test cases written in supertest and published the project in github named express-route-bug, please fork/check out the project run "npm test" to validate it.

If you find its a valid bug, Please report the same to express issues.

I think since the bug is with express itself, Let me know if I still have to write test cases for this issue #140 .

[bajtos]

@pktippa I am afraid I don't see how is your test case in express-route-bug related to the work made here?

Let me rephrase the test scenario from #22.

1. Create a loopback app
2. Setup explorer on this app
3. GET /explorer/swagger/json to ensure any lazy initialisation is executed
4. Add a new model
5. GET /explorer/swagger/json and check that the new model is included

[pktippa]

@bajtos I have completed writing test cases for new model creation and existence of new model in the swagger.json

[pktippa]

@bajtos for your query.

To be honest, I don't see how moving a variable declaration (not initialisation!) to a higher scope solves the problem.

Please go through the conversation below and explanation given by @dougwilson
expressjs/express#2822

[pktippa]

Hi @bajtos can you please look into the test cases and accept this PR.

[bajtos]

You should not need to call explorer(app) again, lines 274-275 should be removed as well.

When you call explorer(app) again, it will add a duplicate set of routes to the application. Each express route incurs a small performance penalty, therefore adding unnecessary duplicate routes can cause performance degradation.

Also the code adding new models should not need to be aware of the fact that the app is using explorer. Your current solution creates a coupling between unrelated parts of the system which is a bad practice.

So, please remove L273-L275, I am suggesting to move L269-L271 back to the test method and get rid of createNewModel entirely.

[pktippa]

Hi @bajtos

I have modified the code and tested, please check it once.

Please let me know revert commits and push a single commit for this pull request and let me know the steps as well, since I am new to git and github.

[bajtos]

Hi @pktippa, thank you for the update, the new code looks much better. Unfortunately you have messed up whitespace in your last commit, see diff and diff ignoring whitespace changes. Could you please restore the original whitespace? It will make it much easier for me to comment on your changes.

Please let me know revert commits and push a single commit for this pull request and let me know the steps as well, since I am new to git and github.

You need to run git rebase -i master, then choose "squash" for all commits except the first (top) one. Once rebased, you need git push -f.

Don't worry about it now, it's best to clean up the history only at the very end, once the patch is good to be landed.

[pktippa]

Hi @bajtos

Unfortunately you have messed up whitespace in your last commit,

That happened because of the copying the code and switching editor from notepad++ to eclipse.

Could you please restore the original whitespace? It will make it much easier for me to comment on your changes.

I reverted the previous commit and restored original white space, please have a look at it once.

Don't worry about it now, it's best to clean up the history only at the very end, once the patch is good to be landed.

Ok.

[bajtos]

Hi @pktippa, thank you for fixing the whitespace, the diff looks much better now!

I am afraid I am very tight on time right now and don't have enough bandwidth to finish the review. I ask for your patience, I'll get back to this as soon as my schedule permits.

[pktippa]

Hi @bajtos,

I am afraid I am very tight on time right now and don't have enough bandwidth to finish the review. I ask for your patience, I'll get back to this as soon as my schedule permits.

We are running a private npm using sinopia and I published this fix in our private repo. Looking for this fix to go public :)

[bajtos]

@pktippa to speed things up, I made few remaining changes myself and landed the patch via 9e8a19f. Thank you for the contribution!

[pktippa]

@bajtos thanks for landing the patch. On which node module version this patch will be available.?

[bajtos]

@pktippa sorry for the delay. I have just released loopback-component-explorer@2.3.0 that includes your patch too. Enjoy!

[pktippa]

@bajtos Thanks 👍