Error handling for failed XPath evaluations

[bbarker]

Sometimes, one browser or another provides more useful information in an error. This time, Chromium is the winner:

• Firefox: The expression is not a legal expression.
• Chromium: Failed to execute 'evaluate' on 'Document': The string '/x:record/x:identifier/x:@identifierType' is not a valid XPath expression.

This raises the issue of what our error reporting API should really be for evaluate, and should we duplicate effort already handled by (some) browsers.

[garyb]

There is a "real" XPath library too: https://github.com/slamdata/purescript-xpath

But getting it right has turned out to be hard work 😉. The version in master there doesn't handle some (important) things that are valid XPath. This branch was an attempt to improve that, and also make it easier to construct, and failed miserably at that. We're back to just using String values where we use it in SlamData, but it would be nice to have something that worked properly one day.

[hdgarrood]

I’d advocate doing as little as possible and passing whatever you get back from the browser through to the person consuming this library; trying to smooth over a particular browser’s deficiencies takes us outside the realm of unopinionated bindings, and the more opinionated something is, the fewer situations it will be useful in. People can always write nicer / higher level libraries on top of this one.

[bbarker]

Thanks @garyb - good to know about that, especially for back-end work. Especially good to know, since unlike the readme says, it isn't currently published on Pursuit :-).

@hdgarrood - thanks, also makes sense. What i might suggest, but don't have a very strong opinion of, would be changing the API so that the evaluateX functions return an Either String X, instead of just X, and the String in this case would be something like:

"Evaluation error for XPath `" <> xpath <> "`: " <> errorMsgFromBrowser

Maybe I'm wrong, but that doesn't feel very strongly opinionated. I'm not in a hurry to do this at the moment anyway.

I would like to maybe leave the issue open, and/or add to the README, so people know to at least test their apps using this in Chromium based browsers if they have an issue, as I just did. :-)

[JordanMartinez]

I think this could be documented in the readme or something. Otherwise, I would close this.

[hdgarrood]

Can you provide some reasoning for why you don't think it's worth exposing error messages that browsers produce via this library's API?

[JordanMartinez]

I think I just misread this issue. I thought we were already reporting what the browser's errors were and the question was whether we should do more work to provide a better error message across all browsers. I understood Harry to argue against this, and then bbarker to propose adding some docs to the readme to at least notify people to use Chrome to test their code.

If we're not already reporting the browser errors and this is wondering how we should handle error reporting, then this should be kept open.