chore(encoding/csv): add deprecation to obsolete exports

[timreichen]

• adds a @deprecated tag to obsolete exports in encoding/csv.ts.

[kt3k]

These seem still useful for conditionally handling the errors from encoding/csv. What is the reasoning for deprecating these?

[timreichen]

@kt3k There are four different kinds of export in encoding/csv.ts as of now:

• api exports (parse, stringify + option types)
• string variable exports (ERR_INVALID_DELIM, NEWLINE etc.)
• trivial type exports (DataItem)
• error type exports (StringifyError, ParseError)

Trivial types exports

I don't think we need to have and export trivial types such as DataItem which is Record<string, unknown> | unknown[].
(I think It would be better to drop the Record<string, unknown> support altogether and prepare the data beforehand for less unexpected errors, but I'll open a separate issue on this).

String variable exports

String variable exports are also trivial and easy to reproduce if one needs them. I think they were exported for the test files. Since they are in csv/_util.ts now, there is no need for them to be reexported in the main file.

StringifyError

Most of the std apis do not have "special" errors but use the built in ones, such as Error and SyntaxError. StringifyError does not add anything except change the error name. Since there are no other types errors that are thrown in stringify, it easily can be replaced with a built in Error.

ParseError

ParseError All (or most) other apis in encoding use the raw SyntaxError. So I thought about consistency across mods. Imo it is not necessary to provide line/column information via the error other than in the error message.

Therefore the deprecations.

[kt3k]

DataItem is part of public API. If we try to deprecate this, we need to replace the usage in our public API definitions as well. Otherwise those types are not rendered in doc website and users don't know the parameters types (see https://doc.deno.land/https://deno.land/[email protected]/encoding/csv_stringify.ts/~/stringify

StringifyError and ParseError
Since there are no other types errors that are thrown in stringify, it easily can be replaced with a built in Error.

If a user does other things together with stringify or parse in a single try block, then other Errors might be caught in a single catch (e) {} block. In that case the conditions like if (e instanceof StringifyError) or if (e instanceof ParseError) should be still useful to distinguish csv-related errors from others.

String variable exports

These are used as part of error message. I think the intention of these being exported is that user can check error like the below:

} catch (e) {
  if (e instanceof ParseError) {
    if (e.message.includes(ERR_BARE_QUOTE)) {
      // specific handling for this error
    }
  }
}

[timreichen]

DataItem is part of public API. If we try to deprecate this, we need to replace the usage in our public API definitions as well. Otherwise those types are not rendered in doc website and users don't know the parameters types (see https://doc.deno.land/https://deno.land/[email protected]/encoding/csv_stringify.ts/~/stringify

Yes, that is what I meant. Opened the issue that is in correlation with this #2660.
The type could then be replaced with unknown[].

If a user does other things together with stringify or parse in a single try block, then other Errors might be caught in a single catch (e) {} block. In that case the conditions like if (e instanceof StringifyError) or if (e instanceof ParseError) should be still useful to distinguish csv-related errors from others.

True. But that is in the specific case of a single try/catch block. One can always nest blocks to handle specific errors without an instance check.
My question would be why csv is the only (except toml) apis we provide in std/encoding that does this while others use the native ones? And web apis like JSON.parse also throw SyntaxError.

These are used as part of error message. I think the intention of these being exported is that user can check error like the below:

} catch (e) {
  if (e instanceof ParseError) {
    if (e.message.includes(ERR_BARE_QUOTE)) {
      // specific handling for this error
    }
  }
}

That doesn't make much sense to me. Why not just use a string directly: e.message.includes('bare " in non-quoted-field')?

[iuioiua]

I agree with @timreichen regarding the handling of StringifyError and ParseError. I think standard errors should be preferred.

[timreichen]

How should we proceed here? Should I bring the pr up to date or revert some of the changes? @kt3k

[kt3k]

I'm currently not in favor of any of these deprecations..

I agree with @timreichen regarding the handling of StringifyError and ParseError. I think standard errors should be preferred.

Can you elaborate on why standard errors are better for these cases? @iuioiua

[iuioiua]

Can you elaborate on why standard errors are better for these cases? @iuioiua

To be clear, I think parse() should throw SyntaxError, like JSON.parse(), and stringify() should throw TypeError, like JSON.stringify(). I'm not in favour of the other deprecations. I think they're useful.

If those errors are sufficient for one of the most important encoding APIs in JavaScript, they're sufficient for other encoding APIs. Being consistent with built-in APIs offers familiarity to the developer and is the engineering path of least action.

[timreichen]

I agree with @iuioiua. SyntaxError and TypeError should be sufficient.

About the other deprecations: Do you think it is a good idea to export error messages? There are two established patterns for errors in std and deno:

1. Use native error classes (SyntaxError and TypeError in this case) without exported error messages (as JSON doesn't export any of them).
2. export specific custom error classes deno does in Deno.errors.[someSpecificError] instead of error messages (which I think is overkill for a simple encoding mod like this).

Exporting error messages seems to be like a hybrid and looks more like an anti-pattern to me.

[iuioiua]

Hm... I see the intention in exporting error messages. However, I find it odd and inconsistent with other patterns within Deno and its Standard Library. On second thought, if I had to choose, I think deprecating the exporting of error messages is a good idea, but my opinions aren't strong.

[jsejcksn]

That doesn't make much sense to me. Why not just use a string directly: e.message.includes('bare " in non-quoted-field')? ^

Exporting error messages seems to be like a hybrid and looks more like an anti-pattern to me. ^

@timreichen As you have observed, error messages are not meant to be enumerated values for automated discrimination checks at runtime for informing program logic — they are intended to impart information to a human observer.

Compared to the strong guarantees of things like infallible operations and monadic return types in some other languages — for better or worse — much of JavaScript is simply fallible and exceptions can, and do, occur almost anywhere... which makes exception-handling just as core as any other circumstance in JS for sound and resilient code. The available language features don't make it simple to write concise and elegant exception-handling code without relying on opinionated patterns from libraries, and the pain is exaggerated even further in TypeScript due to things like the lack of formal exception types.

Just as is it important for return types to be able to be discriminated by program code to select the appropriate next procedure, it is equally important for exceptions to be able to be discriminated in order to inform recovery steps in exception handling. Error subclasses are one kind of opportunity for discrimination of exceptions for those who want to deduce the exception's origin — and they typically come at very little cost.

I recognize that subclasses aren't the only pattern, and this isn't directed specifically at you — I just want to provide more perspective on this issue.

[jsejcksn]

ParseError

ParseError All (or most) other apis in encoding use the raw SyntaxError. So I thought about consistency across mods.
...
Imo it is not necessary to provide line/column information via the error other than in the error message. ^

Why remove a useful feature?

Providing the line/column information in the exception allows for an opportunity for the exception to forward potentially helpful information to the provider of the CSV input data: it can be used to inform about the precise position of the parsing error in the source content — which can also be used to slice a contextually-relevant substring from the input for display.

Without this information that becomes impossible, and if bad input is provided, the provider has to manually figure out what went wrong — likely write more code to pinpoint the cause of the parse error, etc.

[timreichen]

Closing PR due to staleness.