Remove position field, add support for options

[wooorm]

This commit adds support for a single options object, that can contain
the current parameters as fields, but also adds support for a cause
error and an ancestors node stack.

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)
• If applicable, I’ve added docs and tests

Description of changes

This PR simplifies the signatures, in a backwards compatible way, by adding support for a new, single, options parameter.

In that options object, a cause error is accepted, which currently can be given as reason, which is exposed on the vfile message, and closes GH-17.

The options object additionally accepts ancestors, which is a list of (one or more) inclusive ancestors of a problem, which is exposed on the vfile message too, and closes GH-16. One ancestor can currently be given as the second parameter.

The options object also accepts place, which is either a point or a position, which is also exposed on the vfile message replacing position, which is breaking, thus closing GH-15. This place can currently be given as the second parameter.

Finally, the origin argument can be passed in options, and I also added fatal, it might be useful (?). Not sure how far to take it.

The reason I want to keep the input API signatures, is so that older plugins won’t break.

Q: I am not sure about the name ancestors. I still slightly prefer stack (see GH-16).

To do: docs.

Closes GH-15.
Closes GH-16.
Closes GH-17.

[remcohaszing]

Wouldn’t it be better to pass reason to the super() call? Anyway, don’t consider this comment blocking.

[remcohaszing]

I’m loving we can use options now. Can we also support actual, expected,url, and note? Currently these are a bit awkward to use, because in vfile, file.fatal() throws, but it is often desirable to set at least url.

[wooorm]

We can go with 3 routes:

a) only a bare minimum of fields, sticking close to Error (cause) but with the addition of specifying who is emitting the warning (source, ruleId) and where in the file/AST it happened (place, ancestors)

b) fields from a) but with the addition of specified fields (file, fatal)

c) all fields are possible, including undocumented fields (say, "whateverData: 123"), well-known fields (actual, expected, url, note), everything is copied in

I think c) is complex because it means:

c-a) data management: are things shallow-copied over or deep cloned? What if someone does new VFileMessage('x', otherVFileMessage)? What if someone later pushes to one reference of an actual array, does that affect the one of the message?
c-b) some fields are reflected from other fields (line from place), what if both are passed?
c-c) some undocumented fields such as line/column/start/end/type are currently used to detect if the passed value is not an options object

The alternative to b) and c) is a simple assign: const m = new VFileMessage('', options); x.actual = ... or Object.assign(x, rest).
I think I’d prefer a), to keep it simple, and because the alternative (simple assigns) is acceptable to me.

[remcohaszing]

I agree c) is too complex for the same reasons you provided.

The main problem with a) is, the alternative isn’t just simple assigns when it comes to usage with fatal in vfile. It involves try-catch logic:

try {
  file.fail('Some message')
} catch(error) {
  const message = /** @type {VFileMessage} */ (error)
  message.url = 'https://example.com'
  throw message
}

This looks nicer IMO:

file.fail('Some message', {
  url: 'https://example.com'
})

I propose d) Support known fields, and make what are currently well-known fields just regular fields.

[wooorm]

Your file.fail example can be solved with the alternative of assigning too:

const message = file.message('Some message')
message.url = 'https://example.com/'
message.fatal = true
throw message

fatal is just a field. file.fail is just a shortcut.

---

Your approach d), which I think is b) but with making “well-known” fields as standard fields, still has problem c-a) for actual and expected.

I do think that “well-known” fields make candidates for becoming normal fields. But for them to be normal, I believe they should be implemented in different unified/vfile tools. url and note are I believe the best candidates for becoming regular fields (and also options). But actual and expected are never used yet (and I’m still a bit unsure how they work).

---

I think it’s easier to go with simple options first (Options as proposed here, w/o fatal) and then have new discussion about whether to make certain fields “standard”, and whether they should be supported in Options?

[remcohaszing]

const message = file.message('Some message')
message.url = 'https://example.com/'
message.fatal = true
throw message

This is way better than the try-catch solution.

---

Currently expected is used by unified-language-server for quick fixes.

I agree the discussion of whether well-known fields should become normal fields is another topic, though it becomes more relevant when allowing an options object.

Also it can be added later anyway.

[wooorm]

Agreed.

In the discussion on actual/expected, I think we need to figure out more on how that should work. Say we have an HTML lint rule that looks for language-md classes on code elements and warns that it should be language-markdown instead. It emits a warning on the entire code node.

So the range would represent:

```md
# hi
```

actual would be 'language-md', and expected would be ['language-markdown'].
Or not? And what about #13?

Feels like there’s a lot we need to figure out before it should be “standardized”

[remcohaszing]

I think technically cause may be any value, even falsy ones. The MDN page even uses non-error causes in examples.

[wooorm]

That’s the case for plain errors, if we subclass those, we could force a tighter type, right? I went with that, as we can always loosen it up if needed later, and I don’t think we currently need any or unknown there?

[remcohaszing]

Subclass types need to be assignable to that of the parent class, if which it is unknown. Using a different type currently only works, because Error is not declared as an actual class by TypeScript, but like this in the es5 types:

interface Error {
    name: string;
    message: string;
    stack?: string;
}

interface ErrorConstructor {
    new(message?: string): Error;
    (message?: string): Error;
    readonly prototype: Error;
}

declare var Error: ErrorConstructor;

Error is augmented in the es2022 types like this:

interface Error {
    cause?: unknown;
}

I feel like typically cause should be an error. Also it works for now, and the type can be loosened later. So I’m fine with it, just be aware that it could become problematic later.

Edit: It actually already is of type unknown. Hover over custom.cause in this playground link.

[wooorm]

Is there still a problem? As far as I am aware this class is assignable to its Error super even if TS implemented that as a class.

[wooorm]

I think your playground has a typo on L13: you meant to construct CustomError instead of Error?

[remcohaszing]

That typo totally makes the difference. 🤦

I’m still leaning towards unknown, but it’s not a strong opinion anymore.

The problem still stands that subclass properties should technically match their parent’s and this only works because Error isn’t declared as class.

I.e. in this updated playground

class CustomError extends Error {
  cause: string

  constructor() {
    super()
    this.cause = ''
  }
}

class ExtendedCustomError extends CustomError {
  cause: unknown
}

const error = new Error()
error.cause

let custom = new CustomError()
custom.cause

const extended = new ExtendedCustomError()
extended.cause

custom = new ExtendedCustomError()

[wooorm]

I don‘t know what this new playground means.
You are talking about if error was a class.
Here is an example of something being an actual class, using unknown, and a subclass using string: https://www.typescriptlang.org/play?target=99#code/MYGwhgzhAEAqAWBLAdgc2gbwFDWsMArhAKYBc0ByA1sgPYDuyWOetyEALgE4HAe1cAFAA8A-OU5cU6AD7RkBALYAjYl2hyFIEBorIAJsQBmKYvoCUmFrg5IIAOnxFi0ALzRhLAL5YfzUJAwAMJE-IoI0tDEwhzEBjARaFa4TiQS3NLMKWySvPxCYulSaJbYuLgQBAAOaoLm1tC2iA6pLu7CGnIA5F3evv45HNBgbvLE9HBIaHVYYI6EJAPsQ8qjyOPQIZy04VOoM8rzzlhAA.
I believe this shows it is fine to have a specific value in something that extends a vague value.

[wooorm]

I think your example with ExtendedCustomError having a vague value (unknown), and CustomError having a specific value (string), is exactly what‘s not happening in this case (VFileMessage having a specific value, Error having a vague unknown value)?

[remcohaszing]

I see I messed up. I should have swapped the cause types in my example, which makes it without errors. Also the initializer matters apparently.

Then I have no objections to keeping this as-is.

[wooorm]

Perfect! We can always make it unknown later if needed!