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?

Would someone give me a response to this at any time? THX

@slnode ok to test

@bajtos Thanks for your response.
I think the problem occurs here: https://github.com/strongloop/loopback-boot/blob/2.x/lib/compiler.js#L693

Is a unit-test for this needed yet? getExcludedExtensions is anyhow not tested in the current 2.x ...

@bajtos I cannot get any information about why theses 2 tests has failed. If you want me to change something at the code-base, let me know.

@bajtos You are certainly busy, but I want to ask you to have a short review, merge and release this, because I and maybe other have to create workarounds to solve this problem. I would be very happy you could do this soon, because this problem affects a productive application in my case.

Thanks and best regards
Dominik

@dtomasi sorry for the delay, I was on a vacation.

I think the problem occurs here: https://github.com/strongloop/loopback-boot/blob/2.x/lib/compiler.js#L693

Is a unit-test for this needed yet? getExcludedExtensions is anyhow not tested in the current 2.x

AFAICT, getExcludedExtensions was added by 07d2769, which does include some unit-tests. Could you PTAL if some of those tests can be used to reproduce the problem you are seeing?

I added console logs to fixFileExtension to understand better what's happening under the hood. Looks like the logic in fixFileExtensions is flawed and it will treat any file with .js.* suffix as if it should have been preferred over the .js file. Your proposed changes will therefore fix only one of the possible sources of this bug.

IIRC, the goal of this method is to convert file.json to file.js (or file.coffee or similar, depending on what files exist). The current implementation looks a bit convoluted to me, I think it should be possible to simplify it and fix the bug along the way.

I want to ask you to have a short review, merge and release this, because I and maybe other have to create workarounds to solve this problem. I would be very happy you could do this soon, because this problem affects a productive application in my case.

I don't want to land a change that's not well understood, because such changes usually increase the maintenance costs.

As a workaround, until a proper fix is created and landed, you can fork loopack-boot and then e.g. change your package.json to install your fork (npm can install from git).

@bajtos Hope you enjoyed your vacation :-)

I don't want to land a change that's not well understood, because such changes usually increase the maintenance costs.

I absolutely agree in that. I will have a look the next few days and try to provide a proper solution for that.

@dtomasi hello, what's the status of this pull request? Are you still keen to work on this fix and get it finished?

@bajtos Hello, sorry ... I have currently not status updates. Was too busy the last weeks to work on it and also need some time to plug in first.

@bajtos Hopefully I can have a look next week. Sorry

@dtomasi no worries, take your time 😄

@dtomasi Ping, are you still keen to get this pull request finished? Should we close it as abandoned?

Can one of the admins verify this patch?

@bajtos This branch has been working for us on our apps. If it is abandoned is there any chance this could be merged and the task of cleaning up the implementation be backlogged?

@bajtos Sorry for the delay. I´m currently really busy at work. Cannot find some time to go on with this PR. Think we should close this as abandoned.
Best Dominik

This branch has been working for us on our apps. If it is abandoned is there any chance this could be merged and the task of cleaning up the implementation be backlogged?

Let's be realistic: if we backlog the cleanup then it will never happen.

Considering the situation, I am ok to land this fix if somebody can add a unit-test to verify it. Can you @zbarbuto take over this work and write the unit-test? It should fail in the current 2.x branch and pass with the fix in place.

I think the .map entry should be also accompanied by a comment explaining that it's a partial fix for a bigger problem and pointing to #245 (comment)

Closing in favour of #263