Sourcegraph: Remove sourcegraph.com fallback logic

Created on 4 Apr 2019  路  2Comments  路  Source: sourcegraph/sourcegraph

The logic to fallback to sourcegraph.com does not work properly and adds a lot of complexity that affects the entire browser extension.

Problems:

  • It is implemented in the lowest layer, the GraphQL request function. Higher layers therefor don't know if a request succeeded by retrying on sourcegraph.com. This results in part of the app "just working" with the fallback and the other part thinking the repo exists properly on the configured instance, e.g. the "view on sourcegraph" button #3232
  • performRequest() may throw non-Error instances (the response object) depending on specific GraphQL response fields. The conditions for this are so complex that it is near impossible to know as a downstream consumer if a call may throw an Error (like RepoNotFoundError), throw _the response object_, or return the response object with null fields. Bugs from this are guaranteed and it makes proper error handling very difficult to verify.
  • Since exceptions are not type checked in TypeScript, this would be hard to refactor
  • It doesn't make sense to clean up all the messy error _handling_ in the browser extension if the error _throwing_ is so messy (we would just code to the bad error flow, and have to change it all again later)

Proposal:

  • We replace backend.ts with sane API client functions that do not do fancy fallback logic (see webapp), never inspect GraphQL fields and never throw non-Error instances

    • The queryGraphQL() function should only throw if the request fails, it returns { data, errors } raw from GraphQL

    • The higher-level functions like resolveRev() inspect the result and can throw specific errors.

  • We clean up error handling to work with this predictable error flow
  • Later, we can add fallback properly on the right layer: Checking explicitly once where the repo exists, then passing the repo location around.

cc @sqs

browser-extension bug debt

Most helpful comment

Agreed, customers have not been told to rely on this and it rarely works as expected anyway. Let鈥檚 remove it and bring it back later when it is going to work really well.

All 2 comments

cc @beyang I think this logic was to support one of your customers?

Agreed, customers have not been told to rely on this and it rarely works as expected anyway. Let鈥檚 remove it and bring it back later when it is going to work really well.

Was this page helpful?
0 / 5 - 0 ratings