throw error when before middleware returns undefined

[capaj]

I'd think this would be safer for most programmers who are like me and forget the return

[ianaya89]

Is not a bad idea, I think we can add a warning at least. We can consider same for after mid. @gillchristian @ndelvalle any thoughts?

[gillchristian]

Yup, I've been thinking about warnings. I don't know if throwing is the way to go, but a warning surely is.

We could try something similar to what React does with propTypes checking only on development build.

[ndelvalle]

Yeah, I don't think that throwing is the way to go. A warning sounds good, but just for a dev build like @gillchristian suggested. The thing is that if we do it, we should do it for all functionalities, cause users that use this dev build will expect warnings for all functionalities used in a wrong way.

[ianaya89]

Makes sense, I like the idea, will improve devs experience and is a way to guide new devs how to use the lib. I would like to start working on this.

[gillchristian]

Awesome @ianaya89 !!!

Here is what I mean't about React doing stuff only on the development build. As I understand this can be done with rollup & browserify too, if people ends up using other bundler.

[capaj]

@gillchristian is there a usecase when returning undefined is a valid value? Seemed to me there was not.

[gillchristian]

• before: expected (the last one*) to return the config for the request.
• after: is up to the user consuming the response. Maybe not even a warning necessary 🤔
• finally: does not matter because middlewares are just run one after the other, no params are passed, nor is expected for them to return something.

---

(*) Let's consider the next case:

const noOp = () => {}

const overwriteConf  = () => ({
  headers: { 'Content-Type': 'application/json' }
  mode: 'no-cors'
})

trae.before(noOp)
trae.before(overwriteConf)

noOp runs first and sets the config object to undefined, overwriteConfgets that undefined "configuration object" but does nothing with it, instead it returns a proper configuration object.
I know this is a silly example, I don't imagine people doing stuff like this but it still shows that validation needs to be done to the returned value of the last middleware applied.

[gillchristian]

@ianaya89 check this out. It follows up with the wepack's DefinePlugin I sent in the previous comment.

[ndelvalle]

It would be nice if we dont even have statements like the following in the prod bundle.

if (process.env.NODE_ENV !== 'production') {
  // check and do some warnings 
}