Add some error messages for the developer when .service.js is malformed #1894
Conversation
When loading `.service.js` files which don’t contain services, such as when the export is forgotten, print helpful error messages. This will only occur during development; the unit tests will catch these problems well before code reaches the server.
Generated by 🚫 dangerJS |
services/index.js
Outdated
| serviceClasses.push(service) | ||
| servicePaths.forEach(path => { | ||
| const module = require(path) | ||
| if (!module) { |
There was a problem hiding this comment.
I don't think this is working as you expect. A module that exports nothing gives module the value {} here but in that situation this path is not taken because {} is truthy in javascript. Also if we explicitly module.exports = [], [] is also truthy, but we would want to follow this path in that situation too.
There was a problem hiding this comment.
Ahh, yes, good point.
| } else { | ||
| for (const serviceClass in service) { | ||
| serviceClasses.push(service[serviceClass]) | ||
| for (const key in module) { |
There was a problem hiding this comment.
This assumes that if we've got something that isn't instanceof BaseService we've got an iterable, but if I declare a service file that starts
module.exports = class MyService extends SomethingThatDoesntExtendBaseService...we'll get to here and try to iterate MyService, which doesn't make much sense.
There was a problem hiding this comment.
That's true, though is not extending BaseService a use pattern we need to support?
There was a problem hiding this comment.
No - we don't want to support it. We want to throw an error. That's the point of testing against (serviceClass.prototype instanceof BaseService) - right?
but because MyService isn't iterable we never enter the loop so nothing ever gets tested against the condition (serviceClass.prototype instanceof BaseService) in this case.
There was a problem hiding this comment.
Ah, I gotcha.
What if we accumulate the valid services found in each file, and throw an error if none are found?
There was a problem hiding this comment.
You'd still want to continue checking as you go, or also throw if
num valid services != num total services
to also cover the condition where >=1 services extend BaseService and >=1 don't, but yeah.. if we get to the end of that loop and have 0 valid services that's definitely an error.
There was a problem hiding this comment.
We're right now supporting both arrays and objects, and if we keep doing that, it seems simpler to bail if we ever find a non-service, and check that each file has contained at least one before moving on to the next one.
Alternatively we could drop support for objects and require a single service or an array. Then we could use count, validate in map, and probably simplify this.
There was a problem hiding this comment.
In the relatively small subset of services we've refactored so far, we've already found valid uses for multiple export styles:
object:
shields/services/apm/apm.service.js
Lines 122 to 126 in 67193ce
array:
class:
All of those are sensible module export styles and I think we should continue to support all of them so we keep things relatively flexible for future contributors and allow ourselves and contributors to write idiomatic code. Even if that complicates the service loading code a bit, I think that's worthwhile.
We can detect a module which exports a single class from one exporting an Array or Object using instanceof Function:
> class FooBar {
... }
> console.log(FooBar instanceof Function);
true
> console.log({} instanceof Function);
false
> console.log([] instanceof Function);
false
and special-case it.
|
This doesn't feel like a high priority for an automated test, though I see where you're coming from. If it seems important to you, I'll do it. Or if you feel like tackling the tests, go for it! |
|
@chris48s What do you think about merging this? The existing files should prevent regression on the existing use cases, and we could manually test if this code ever gets touched again. It's kind of a pain to test and I feel like the time could be better spent elsewhere. (This is having just spent a couple hours writing tests for #2021 because I want to get that merged…) |
|
I don't mind merging it without tests if its a complete pain, but either way we should fix those two cases noted in the review where we're not throwing on malformed exports. That said, having had another look I do have an idea for how to write tests for this. Let me have a quick go at it.. I'm happy to pick it up and push some commits to this branch (if I can write to it) or open a new PR to take it over the line.. |
|
Awesome! You should be able to write to the branch. |
- allow us to inject servicePaths for testing - throw a custom exception - add test suite and fixtures - cover off missing cases
|
The new commit I've pushed covers every case I can think of. Does that seem reasonable? |
|
Looks awesome! I can’t 👍 this since it’s my PR. Would you like to merge? |
When loading
.service.jsfiles which don’t contain services, such as when the export is forgotten, print helpful error messages. This will only occur during development; the unit tests will catch these problems well before code reaches the server.