solid-client-js icon indicating copy to clipboard operation
solid-client-js copied to clipboard

Added flag to control diagnostic output on errors

Open pmcb55 opened this issue 4 years ago • 4 comments

New feature description

Added flag to control diagnostic output on errors (as this output can be distracting when errors are expected).

Checklist

  • [X] All acceptance criteria are met.
  • [X] Relevant documentation, if any, has been written/updated.
  • [X] The changelog has been updated, if applicable.
  • [X] New functions/types have been exported in index.ts, if applicable.
  • [X] New modules (i.e. new .ts files) are listed in the exports field in package.json, if applicable.
  • [X] New modules (i.e. new .ts files) are listed in the typedocOptions.entryPoints field in tsconfig.json, if applicable.
  • [X] Commits in this PR are minimal and have descriptive commit messages.

pmcb55 avatar Oct 20 '21 23:10 pmcb55

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codesandbox-ci[bot] avatar Oct 20 '21 23:10 codesandbox-ci[bot]

Yep: "What's the use case for suppressing part of the error message (what situations are the errors expected)?" The diagnostic part of the output contains the entire contents of the dataset as Markdown, so it's very noisy (it's useful output, generally, hence the flag is defaulted to 'true'). An expected error case is when doing ETL - we want the Loading-to-a-Pod process to be idempotent, so a resource may already exist (in which case we add any new triples to that resource), or may not exist at all - so a 404 error could be expected, normal behaviour.

"Should this flag be added to internal_defaultFetchOptions, or is it only relevant for some requests?" Yep - absolutely. It was just that TypeScript was complaining when I tried to do that - whereas this code 'just worked'. But I'd be very happy to rework to simplify.

pmcb55 avatar Oct 21 '21 12:10 pmcb55

Just marking this PR as draft, as after discussion with Andy today, we can make this cleaner by added the new flag to the internal_defaultFetchOptions object, but we should also add a proper TypeScript type too, and then change all references to typeof internal_defaultFetchOptions to use the type instead, and then use spreads to pull defaults, e.g.,:

  const config = {
    ...internal_defaultFetchOptions,
    ...options,
  };

Some places also use options: Partial<typeof internal_defaultFetchOptions>, which means all properties are optional. That means we could use that more widely and don’t necessarily need to create a separate ‘type’ (but we do still need to make sure everywhere handles missing values for fetch with the config). So given there's a few options, and it needs careful consideration, flipping this to 'draft' (but the SDK could still merge this code to get it working now, and then tidy up everything properly in a separate ticket).

pmcb55 avatar Oct 21 '21 15:10 pmcb55

This might be better solved by having a custom error type with a captureDiagnostics() method on it, which would then generate the extend output; keeping error messages smaller by default. (That would also put the control in the developers' hands, and would mean potentially sensitive information wouldn't end up by default in error reporting tools like Sentry.

ThisIsMissEm avatar Nov 24 '21 20:11 ThisIsMissEm

Closing due to this being heavily out of date, and before we add anything similar, we first need a Design Document for the feature.

ThisIsMissEm avatar Nov 18 '22 20:11 ThisIsMissEm