CreateExpressMiddleware: Send 404 code for disabled feature

[jwicks31]

I'm not sure if this is the correct way to handle it, but I think we need to send the 404 status after setting it. Otherwise the status can be overwritten in the handler.

[jwicks31]

Should we set the status here at all if the feature is enabled?

[ericelliott]

Maybe the line that calls this should be:

if (!isActiveFeatureName(requiredFeature, updatedFeatures) {
  return res.status(404).send();
}

Don't call the handler. Don't call next().

[ericelliott]

See comments.

[ericelliott]

LGTM, feel free to merge.

[kennylavender]

Is this supposed to solve the 404 with nextjs? Seems like this would send the correct status, 404, but the page would be empty and this doesn't allow for any HTML to be sent, unless I am mistaken?

[ericelliott]

Looks like you're right. We need to take the 404 page as a parameter, and send it with the .send() call.

[ericelliott]

We need to take a 404 page to send with the response.

[kennylavender]

Instead of taking a 404 page we could take a disabled handler to be more flexible. I don't think that will work with nextjs though, I think the only way to to get nextjs to render a 404 is to set the status from getInitialProps like you pointed out in the other repo. So maybe we can create a nextjs specific HOC for feature toggles.

[jwicks31]

@kennylavender could you help me with what you mean by accepting a disabled handler?

Will this need to behave differently for routes that render pages than for routes that are meant to send JSON responses?

Something like:

if (!isActiveFeatureName(requiredFeature, updatedFeatures)) {
      res.status(404);
      if (req.accepts('html')) return res.render(404Page,  { url: req.url })
      else return res.send({ error: 'Not found' });
    }

[kennylavender]

@jwicks31 our middleware shouldn't know about accept types, that responsibility should belong to the provided handlers.

One way we could allow the user to provide a disabled handler is to have them provide an enabled and disabled handler for each method.

const features = []
app.use(createExpressMiddleware({ features }, 'help', {
  get: {
    enabled: () => {},
    disabled: () => {},
  }, 
  post: {
    enabled: () => {},
    disabled: () => {},
  }
}))

I don't really care for this idea though, as a user, I would not want to do this. In my opinion this middleware needs to be simplified/adjusted, if I remember correctly that is why I did not document the middleware, I didn't want people using it as is.

I wrote a feature toggle middleware for one of the po.et projects that I think was better, I can't remember which project though. I will see if I can find it or write out a new example.

[kennylavender]

Can't find the middleware in the po.et projects.

But I think I would do something like this

createExpressHandler(disabledHandler: Function) => (enbaledHandler: Function) => ({ requiredFeatures: [ ...String], features: [ ...Feature ]}) => void

The user can then use the more specific app.get, app.post etc. and they can still use app.use when their handlers do the method switching themselfs.

import { createExpressHandler } from '@parralleldrive/feature-toggles'

const notFoundHandler = (res, req, next) => {}

const todosHandler = (res, req, next) => {}

const features = []

app.get(
  '/todos',
  createExpressHandler(
    notFoundHandler,
    todosHandler,
    { requiredFeatures: ['todos'], features }
  )
)

I used non-curried format for simplication of the example, but in curried format I think this would be very nice.

[ericelliott]

That looks pretty good. If we auto-curry, it would be trivial to configure a featureOr404 handler.

Why is requiredFeatures an array in this example? If features can have dependencies, couldn't you create a single feature name that encapsulates all of the dependent features?

[kennylavender]

That looks pretty good. If we auto-curry, it would be trivial to configure a featureOr404 handler.

Yup!

Why is requiredFeatures an array in this example? If features can have dependencies, couldn't you create a single feature name that encapsulates all of the dependent features?

Yup, that works too. I think I was favoring avoiding dependencies and keeping features flat. One could name a required feature set for a new feature and then provide it to the function. This is something I will explore more in another issue.

[ericelliott]

Yup, that works too. I think I was favoring avoiding dependencies and keeping features flat.

The alternative is that you then have to keep track of the feature graph everywhere you use features throughout the app, which multiplies the complexity.