enhance-dev/arc-plugin-enhance

Should we enforce API routes return Object with keys?

kristoferjoseph opened this issue · 6 comments

Just because:

> var f = Object.assign({}, ['foo', 2, 3])
{ '0': 'foo', '1': 2, '2': 3 }

> var wut = JSON.stringify([1,2,3])
'[1,2,3]'

> var vale = JSON.stringify({ "a": [1,2,3]})
'{"a":[1,2,3]}'

> JSON.parse(wut)
[ 1, 2, 3 ]

> JSON.parse(vale)
{ a: [ 1, 2, 3 ] }

IMHO, API routes should always return JSON Objects instead of Arrays. Returning an object is a best practice as you can always extend your API by adding more values to the object whereas switching from an array to an object is a breaking change.

[
  { "name": "Bob Roberts", "age": 48 },
  { "name": "Dan Slott", "age": 36 }
]

{
  "users": [
    { "name": "Bob Roberts", "age": 48 },
    { "name": "Dan Slott", "age": 36 }
  ],
  "results": 2,
  "pages": 1,
  "offset": 0
}

Back in the day, there was a security vulnerability in return a JSON array but I believe most browsers have patched this issue.

I tend to agree with you @macdonst
Let's add this restriction.

I agree for a new API. Only problem with that is what if you are trying to replace or replicate an API that does it wrong (i.e. returns an array).

It's not uncommon for an API to return a scalar value like true so I'd rather not add this restriction esp as no one tmk has asked for it (?)

You are right no one has asked for this limitation. The problem is the API can emit scalar or Arrays NP, but Enhance, in the corresponding page will convert that into an object. It doesn't fail but it would be a source of unexpected behavior and probably bugs. Enhance could be updated to accept Arrays or scalars easily. But the real problem will show up downstream when we want to connect to the client side store. The store depends on keyed so it can't accept an Array. To maintain that client/server store symmetry for the future I think we should:

  1. Not restrict the API. Let it emit arrays etc. in case it is not using Enhance.
  2. Make Enhance throw an error if you try to set initialState as anything but an object.

yeah that feels like a more correct solution (to me)