ssrwpo / ssr

SSR - Router with SSR for Node & Meteor
https://ssrwpo.github.io/ssr/
MIT License
93 stars 16 forks source link

Cannot read property 'title' of null #59

Closed s7dhansh closed 7 years ago

s7dhansh commented 7 years ago

If I provide a route with allowed query params in the server routes, and then try to open that route link without any query parameters, it results in an error:

Cannot read property 'title' of null

with server returning a 500 error.

I can fix it by using: if (!Object.keys(query).length) return {};

        urlQueryParameters: (params, query) => {
            if (!Object.keys(query).length) return {};
            const allowedKeys = ['abc'];
            const allowedQueryParams = pick(query, allowedKeys);
            if (Object.keys(allowedQueryParams).length !== allowedKeys.length) return null;
            return allowedQueryParams;
        },

I think checking the empty query should be the default behavior in this package. Or at least, a proper error must be thrown.

PEM-- commented 7 years ago

Indeed, it's up to the package user to take care of ensuring that their URL can be accessed with or without URL query parameters. Piling up checks into this package would end up in performance loss.

s7dhansh commented 7 years ago

@PEM-- in that case, at least there should be a better error, right? That error is not at all helpful, and emerges from probably some parameter being null in this particular case.

PEM-- commented 7 years ago

Cannot read property 'title' of null comes from your app not from the package. Unless I didn't get your use case, we can't prevent mistakes in user's apps.

What could be done is, instead of simply returning an array, this package could provide an object that package user would fill with their error management. Something with a contract like:

// When there's no error
{
  res: allowedQueryParams, // An array of allowed URL query parameters
}
// When there's an error
{
  httpCode: 404, // The error code the package user would like
  httpReason: 'title is a mandatory URL query parameter', // A reason for it
}
s7dhansh commented 7 years ago

Sounds good.

I do not think that the error is because of my code. There is no .title in the code which would cause this. I can not find any in this package too. The only option remains is that the error is emanating from react or any other package. Will dig deeper sometime soon.

PEM-- commented 7 years ago

PR are welcomed and encouraged 😉

I'm closing this as it's more a question than an issue.