Is it possible that in some environments the application will not be allowed to write to disk? Or that the contents returned is not JSON parsable? In these cases it would throw errors, do we want to wrap these operations in a try/catch? Handling errors for file reading and writing and JSON deserialization is generally the practice in other languages (e.g. Java, Kotlin, Go), and it's probably a good idea in the Node SDK to avoid crashing the user's app.

Related, I see we are wrapping all the middleware operations in a try/catch but if it fails, it will call the next() function with an error—wouldn't this skip the request handler and go straight to any error middleware, resulting in the request to fail? Do we want to fail requests when networking or cacheing-related disk I/O operations fail?

If you pass anything to the next() function (except the string 'route'), Express regards the current request as being an error and will skip any remaining non-error handling routing and middleware functions. (ref)

Good catch with the filesystem operations. I'm still thinking through the general error handling part. Not happy with how it is now. I think some people will want to customize the behavior when loading features doesn't work (e.g. use our CDN first, but fall back to a static file in case the CDN is down).

Why are we requiring a Request to be passed here? I'm not sure which common Node.js framework this would work with, but in Express, when passing data through the request lifecycle, the recommended pattern is to use the response locals, not the request object (though I know in our backend code we are not following the recommended pattern and have instead augmented an Express request).

Shouldn't they be able to pass anything in here? Or nothing? They just need a function that returns the attributes, we don't need to enforce the arguments it takes.

We can do something like this instead:

interface GetAttributes {
    (...args: unknown[]): Promise<Record<string, unknown>>
}

This will make it more flexible for the developer's implementation. Of course it means they have to do type assertions but it will not constrain them to a specific API.

Example without arguments

const getAttrsNoArgs: GetAttributes = async () => {
    return {
        foo: 'bar'
    }
}

Example using Express' res.locals pattern

// naive express response type
type ExpressResponse = {
    locals: Record<string, unknown>
}

const getAttrsWithRequestAndResponse: GetAttributes = async (req, res) => {
    const method = (req as Request).method

    const locals = (res as ExpressResponse).locals
    const foo = locals.foo

    return {
        method,
        foo,
    }
}

More complex implementation

This one has a UserRepository:

type User = {
    id: string
    email: string
    registeredAt: string
}

class UserRepository {
    async getUser(): Promise<User> {
        return {
            id: 'user-abc123',
            email: 'user@example.com',
            registeredAt: '1689026908',
        }
    }
}

const getAttrsWithUserRepo: GetAttributes = async (userRepo) => {
    return await (userRepo as UserRepository).getUser()
}

Naming it GetAttributesFunction might be more inline stylistically for JS/TS than GetAttributes.

You can see a TypeScript playground here.

Not sure I follow. The middleware is defined once and called for every request. So the getAttributes function needs context about which request it's getting attributes for (e.g. to look up cookie values). Since we're calling the getAttributes function from within the middleware, we need to know exactly which arguments to pass in, so we can't let the user define their own.

My understanding would be that the user would define the getAttributes function themselves, and the only requirement is that it return attributes as Record<string, unknown>. If previous middleware has populated the response locals with relevant data, for example, they wouldn't be able to get them and would need to adapt their app to work with this middleware. This may require them to make another round trip to the DB which wouldn't be ideal. I'm thinking in the case that perhaps they have auth middleware that populates the response locals (as is generally done with the patterns recommended by Express), then if they need to get any data from the response locals, they cannot do it in TypeScript because they are constrained to working with a Request object only.

For example, this wouldn't pass type checks in TypeScript:

    const middleware = growthbookMiddleware({
      context: {},
      getAttributes: (res) => {
        return {
          registeredAt: res.locals.registrationDate
        };
      },
    });

It seems like we're unnecessarily constraining them to defining things on the request object by enforcing the request be the only argument. It doesn't seem we actually need it to be a request since the user defines the function themselves.