inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.33k stars 718 forks source link

Don't throw `Invalid return type in middleware` in case of possibly `undefined` value. #1196

Open ValeryLosik opened 4 years ago

ValeryLosik commented 4 years ago

Expected Behavior

Should not throw error if some middleware is applied and bound value is undefined | null or optional.

Current Behavior

Throws error if some middleware is applied and bound value is optional or undefined | null.

Possible Solution

Don't throw error in case of undefined | null result returned from the middleware.

src/container/container.ts

        if (this._middleware) {
            result = this._middleware(defaultArgs);
            if (result === undefined || result === null) {
                throw new Error(ERROR_MSGS.INVALID_MIDDLEWARE_RETURN);
            }
        } else {
            result = this._planAndResolve<T>()(defaultArgs);
        }

Steps to Reproduce (for bugs)

Link to example

  1. Create container
  2. Bind any undefined value
  3. Apply some middleware
  4. Try to get value from the container

What do you think about it? Can I make a PR for it? Thanks in advance.

fljot commented 4 years ago

~There's not very much info on middleware design and usage, but this one looks indeed like a bug to me because even basic middleware~

const basicMiddleware: interfaces.Middleware = planAndResolve => nextArgs => planAndResolve(nextArgs);

~would break optional injections.~

Oopsy. No, such middleware still allows use optional injections.

Nevertheless, I believe that this error is not needed anymore. I've looked at the git history, and looks like original INVALID_MIDDLEWARE_RETURN made sense back when it was introduced in 3e7b81806f7c8e6b3c4f7dfaa27ac6afe4ecdbf2 – that error was saying "Invalid return type in middleware. Return must be an Array!", and indeed it was legit because it was produced only from _get, which back at that time was used only for multi-injections, so reminder to return an array could be useful. But later in #392 resolution process was refactored and calls to middleware were consolidate in refactored _get, which began to be used for all resolutions (container get- methods) https://github.com/inversify/InversifyJS/pull/392/files#diff-20bba02a2dfc0a7fc56f1b31be7e8425 So now _get through middleware doesn't looks any different from regular call to _planAndResolve (it actually is the initial call in the chain of middlewares) image image