[v2] Invalid paths should warn not throw

[madeleineostoja]

Invalid paths (eg: //some/path) currently throw errors. They should console warn and bail out of the request instead. You would expect a dodgy request to just not work, rather than blow your stack.

Thoughts @bedeoverend? Could argue it's up to component authors and SDK consumers to validate their inputs, but again I think that we're overreacting in terms of consequences atm.

[bedeoverend]

I think it should throw - this kinda of thing will most likely come up during Development rather than in production, and having an error encourages people to fix it / also to handle errors properly, which should be being done regardless.

Also, I see warnings as, this will work, but you should do it differently in future. Errors say, this won't work, don't do this again. Think of it like this:

Simpla.set('//foo', { data })
  .catch(err => /* Handle Error */);

It would be really weird if that request didn't go through, and the error handler wasn't called. If it didn't work, I would expect it to throw an error back at me.

[bedeoverend]

I think it'd be ok if we did perform the request still e.g. if we accepted odd paths and just normalized them, as discussed on paths. Then just throw errors or UIDs that are ambiguous which we can't normalize

[madeleineostoja]

Not all paths be normalized, just the input to SimplaPaths.observe() because the format of the basePath argument is ambiguous.

In general cases, given //path/tail, /{undefined}/path/tail and /path/tail are very different interpretations and could well result in the wrong data being fetched and therefore FOUC, buggy requests, etc.

I don't see how the use-case outlined here is 'just in development'? Parts of a path going momentarily undefined that is.

[bedeoverend]

If that's the case re: confusing basePath, I'd say we should just document default basePath equals ''. And then if you provide one, it should be like /foo.

I'm very much against it warning rather than throwing, I think that's odd behaviour - at least in the core SDK. Maybe we can catch and warn inside elements?

But if I'm doing Simpla.get(path).catch(handleError) on the SDK, I would expect it to get the value, if it doesn't get the value, I'd expect an error to occur explaining why, and I'd expect to be able handle that error - if I give a function invalid input, I'd expect it to error.

Or another way, think of if you had this code:

Simpla.get(invalidPath)
  .then((response) => {
    // What happens here?
  })
  .catch(handleError);

That response handler, should it get called? If not, there's no way I can handle it failing, and if it does - what does it get called with?

I think maybe the resolution here is to put good error handling in the elements in case they've got invalid paths, given the devs might have less control over what gets set on the path...?

[madeleineostoja]

Well in that case shouldn't it just make the request, and then return (assumedly) a 404 or something? Rather than blowing the stack?

I don't know, it just seems a very easy gotcha to fall into.

[madeleineostoja]

And yeah makes sense for elements to handle this normally, but I'm really wary of just offloading stuff to elements as a solution, because it just more and more boilerplate burden for element authors, raising the barrier of contributing to Simpla's ecosystem. There's already too much boilerplate crap they have to deal with for my liking.

[bedeoverend]

Re: first comment - so a 404 would do the same thing, it would throw an error - as it's a bad user request - it would just perform the network request first. It's standard, devs should handle the errors.

As for element boilerplate, yeah I think we can improve it in future - I think making things like mixins for elements authors will help. And just documenting solid patterns to follow. Having said that, I certainly don't think it's unreasonable for now.

[madeleineostoja]

Okay, maybe I'm misunderstanding something then - a 404 wouldn't blow the stack would it? It would just be a bad request in their console? Anyway, I actually don't feel too strongly about this, so happy to keep it erroring and throw it in the boilerplate for now.

For the record though, I really don't think mixins and crap are the right answer to what is quickly becoming a complicated process to build Simpla elements. All of this stuff should be handled better by the SDK, that's the point of it. If it's hard and confusing to build elements, and everyone has to follow very particular patterns to stop things blowing up in their faces, then no one would build them, and that would be a disaster.

[bedeoverend]

Essentially yeah it would - it would blow it's current promise chain if not caught properly. The alternative would be to not fail and to be more forgiving on the formatting of the given path, that's slightly different to this issue though. Otherwise they couldn't catch errors, unless you wanted to give them more information in the response. That's essentially how fetch works, it doesn't consider network errors as errors, so it gives back a verbose response which includes HTTP status codes etc. so the user can check if it was a bad network request.

I think it's worth keeping an eye on, and can try to improve it as we go. Having an easier system would be great, it's just a hard balancing act to do while maintaining power and flexibility.

[madeleineostoja]

The alternative would be to not fail and to be more forgiving on the formatting of the given path

That's essentially what I'm advocating. I don't think the tradeoff for element authors is strong enough to justify blowing their promise on bad requests - what's the actual use-case for this, versus checking what the response is? Especially if that's how fetch already works...

Again, I don't care too strongly about this particular issue, because it is a bit of an edge case, but I do care very strongly about ease of element authorship. From a business logic (and community and etc etc) perspective we're shooting ourselves in the foot by making element author's lives harder.

[bedeoverend]

What I meant by that, would be to accept more path formats, and still perform a valid request. Fetch is very low level, and works for HTTP responses because you can get back the body of a response, and also header status codes e.g.

fetch('/bad/request/url')
  .then(response => {
    if (is40x(response.status)) {
      // Handle bad request
      // Maybe inspect the response.body() still as we might want to know what's in there
    } else {
      return response.body();
    }
  });

so for us, that would mean adding extra potential information to the response:

Simpla.get('//bad//path')
  .then(response => {
    if (response.status === 404) {
      // Handle bad request
    } else {
      return response.body; // Then would have to store the actual content somewhere else
    }
  });

which isn't the way for us given that a user doesn't need that extra information - fetch does it as you can get a bad request status code and a body from the response.

If there was any arbitrary JS function, and I gave it params that meant the function wasn't going to work - I wouldn't expect that function to just do nothing and not tell me about it. And by tell me about it, I mean programatically give me something to deal with. Warning in the console isn't something the program can deal with.

[madeleineostoja]

Yep fair enough

Did UIDs used to just not complain? All these blown stacks only came up when we stared using paths

[bedeoverend]

Yeah I believe UIDs just didn't say anything, which would mean a bunch of invalid requests - I'm also not sure we were constructed UIDs before? Might be wrong there, worth investigating what we were doing previously and what we're doing now.

[madeleineostoja]

We definitely did. I just went through simpla.io and converted a bunch of templated UIDs to paths