search

wooorm / load-plugin

Load a submodule / plugin
MIT License
15 stars 1 forks source link

Wrap errors in an `AggregateError` #21

Open remcohaszing opened 6 months ago

remcohaszing commented 6 months ago

resolvePlugin tries to resolve a plugin from several locations. Each try may fail, but only the last error is preserved. I think it’s better to collect all errors, and throw an AggregateError instead. This communicates all attempted locations to the user.

If only one error is thrown, perhaps no AggregateError is needed.

wooorm commented 6 months ago

Did you actually run into this? What did you see, what did you expect?

If only one error is thrown, perhaps no AggregateError is needed.

🤔🤔🤔 Well, one error is thrown, so you are saying AggregateError is not needed? 🤔🤔🤔

remcohaszing commented 6 months ago

Did you actually run into this? What did you see, what did you expect?

It took me a minute to understand what’s going on with https://github.com/orgs/remarkjs/discussions/1333. I think an AggregateError would have helped both me and the user understand what’s going wrong.

If only one error is thrown, perhaps no AggregateError is needed.

🤔🤔🤔 Well, one error is thrown, so you are saying AggregateError is not needed? 🤔🤔🤔

Multiple errors are thrown and caught. Currently only one is preserved and rethrown.

If from.length === 1 and plugin is falsy, then only one error is ever thrown. In that situation I’m not sure if it’s useful to wrap it in an AggregateError. There’s something to say for either IMO.

For reference, based on this script:

try {
  throw new Error('a')
} catch(a) {
  try {
    throw new Error('b')
  } catch(b) {
    throw new AggregateError([a, b], 'Oh no!')
  }
}

This is what an AggregateError looks like in Node.js:

AggregateError: Oh no!
    at file:///home/remco/script.js:7:11
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5) {
  [errors]: [
    Error: a
        at file:///home/remco/script.js:2:9
        at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
        at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5),
    Error: b
        at file:///home/remco/script.js:5:11
        at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
        at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)
  ]
}
wooorm commented 6 months ago

It took me a minute to understand what’s going on with github.com/orgs/remarkjs/discussions/1333

Perhaps the tools between load-plugin and the user should throw a better error? Wrap it, and expose the currently thrown error as a cause?

Or load-plugin should throw a better error, with a cause?

Or, you could infer from the error that this was running in Electron and hence the baked in Node packages were checked?

How would you imagine that error to be improved?

Multiple errors are thrown and caught. Currently only one is preserved and rethrown.

What is a bit weird about this is that we do not know which errors are correct and which not. Or, more correctly, we know that up to one error is correct, and all the others are incorrect: e.g., someone is supposed to have say remark-gfm in one particular place, probably not in the other places?

remcohaszing commented 6 months ago

Why aren’t all errors correct? That’s why I propose using AggregateError. It’s basically an error with multiple causes.

So it would look something like this:

- let lastError
+ const errors = []
  …
- lastError = error
+ errors.push(error)
  …
- throw lastError
+ throw new AggregateError(errors, 'Failed to resolve ' + name)
wooorm commented 6 months ago

Why aren’t all errors correct?

This is a very low level tool. It can be called with 'gfm' and 'remark' and './path/to/folder' and './path/to/cwd' and './path/to/global/modules' and './electron/baked/in/modules'. Then it would aggregate 8 errors.

It would generate those 8 errors too when the user forgot to run npm i and so './path/to/folder/node_modules/remark-gfm' was missing.

Another reason this could fail, is when Node is already trying to read X files on some low-resource computer. 7 errors would be about remark-gfm and gfm being missing. One about not being to read some file because ERR_BUSY. Or similarly, someone was running npm install in parallel which changed the format of node_modules, previously files existed but not anymore. The user is not helped by 8 errors.

The reason you raise this issue for a confusing error message, shows that one of the error messages is confusing.

Can you also consider the other questions? I think those are important. I would imagine that you were helped more by a better error message in vscode-remark. Or even a better error message here. Compared to 8 errors.

remcohaszing commented 6 months ago

The user is not helped by 8 errors.

The reason you raise this issue for a confusing error message, shows that one of the error messages is confusing.

I don’t understand. What’s confusing here IMO is that one of these 8 errors needs to be resolved. Which one that is, depends on a context that load-plugin doesn’t have. Each individual error message specifically is fine, but the user only gets to know the last error. Aggregating the errors into one solves exactly that. I don’t see why the user isn’t helped by showing all errors instead of just the last one.

Are you worried having 8 stack traces is overwhelming? Another solution would be to throw a new error, whose error message is all collected error messages joined by a newline.

Perhaps the tools between load-plugin and the user should throw a better error?

Perhaps, but I really do think the individual errors are fine. The problem is that only one error is surfaced, while another error may be more helpful depending on context.

Wrap it, and expose the currently thrown error as a cause?

Sure, load-plugin probably swallows the relevant error. Or perhaps there are multiple errors that can be acted upon. For example installing remark globally or in the project locally can both be viable solutions.

Or load-plugin should throw a better error, with a cause?

Sure, but which error is the cause? This is what AggregateError is for.

Can you also consider the other questions?

I tried, but I really keep circling back to the same problem. All errors are caught, but only one error is thrown. All others are swallowed.

wooorm commented 6 months ago

I don’t see why the user isn’t helped by showing all errors instead of just the last one. Are you worried having 8 stack traces is overwhelming?

Yes, keeping that vscode-remark user in mind, who was confused by that one error stack, now imagine how 4-8 times that error stack would look to them.

Your “Which one that is, depends on a context that load-plugin doesn’t have.” is indeed a good point, and why I believe that that user is helped more by an improved error above load-plugin. That’s my previous point “Perhaps the tools between load-plugin and the user should throw a better error?”. I don’t think we can help that vscode-remark user in load-plugin.

I do agree that the error here could perhaps be improved, but I think improving goes further than just showing 4-8 errors. That’s my previous point “Or load-plugin should throw a better error, with a cause?”. Thinking further on this, most of the errors caught here will likely and expectedly be that remark-gfm is not installed in paths X, Y, and Z, and gfm is not installed in paths X, Y, and Z either. There could also be arbitrary errors, but this is not the common case. So, we could detect if all errors are about gfm/remark-gfm missing in X, Y, Z, and then show a pretty error for that, or alternatively show an (aggregated) error for the arbitrary non-missing errors we get.

remcohaszing commented 6 months ago

I don’t see why the user isn’t helped by showing all errors instead of just the last one. Are you worried having 8 stack traces is overwhelming?

Yes, keeping that vscode-remark user in mind, who was confused by that one error stack, now imagine how 4-8 times that error stack would look to them.

Ok, this makes sense.

Your “Which one that is, depends on a context that load-plugin doesn’t have.” is indeed a good point, and why I believe that that user is helped more by an improved error above load-plugin. That’s my previous point “Perhaps the tools between load-plugin and the user should throw a better error?”. I don’t think we can help that vscode-remark user in load-plugin.

The problem is that the tool above load-plugin doesn’t have this context either. load-plugin determines whether or not it should look for electron or nvm related folders. And in this particular case whether to install remark locally or in the project is up to the user.

I do agree that the error here could perhaps be improved, but I think improving goes further than just showing 4-8 errors. That’s my previous point “Or load-plugin should throw a better error, with a cause?”. Thinking further on this, most of the errors caught here will likely and expectedly be that remark-gfm is not installed in paths X, Y, and Z, and gfm is not installed in paths X, Y, and Z either. There could also be arbitrary errors, but this is not the common case. So, we could detect if all errors are about gfm/remark-gfm missing in X, Y, Z, and then show a pretty error for that, or alternatively show an (aggregated) error for the arbitrary non-missing errors we get.

I’m not a fan of phrasing “with a cause”, because there are multiple causes.

I think we can get quite far by showing an error message like (this can probably be improved):

let errorMessage = 'Failed to load ' + name
if (plugin) {
  errorMessage += ' or ' + plugin
}
errorMessage += 'from ' + from.join(' or ')

throw new Error(errorMessage)

IMO it could still be an AggregateError though. The tool implementing AggregateError can choose whether to show the error message, stack trace, or complete error context. For reference, this script:

import { inspect } from 'node:util'

try {
  try {
    throw new Error('a')
  } catch (a) {
    try {
      throw new Error('b')
    } catch (b) {
      throw new AggregateError([a, b], 'Oh no!')
    }
  }
} catch (error) {
  console.log(error.message)
  console.log()
  console.log(error.stack)
  console.log()
  console.log(inspect(error))
}

yields:

Oh no!

AggregateError: Oh no!
    at file:///home/remco/script.mjs:10:13
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

AggregateError: Oh no!
    at file:///home/remco/script.mjs:10:13
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5) {
  [errors]: [
    Error: a
        at file:///home/remco/script.mjs:5:11
        at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
        at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5),
    Error: b
        at file:///home/remco/script.mjs:8:13
        at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
        at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)
  ]
}

In the case of vscode-remark, I imagine it’s useful to show the error message to the user, and log the full AggregateError including all stack traces in the logs for troubleshooting purposes.

wooorm commented 6 months ago

There are many possible errors, not just errors for unfound packages. Your pseudocode is a fine idea for the “So, we could detect if all errors are about gfm/remark-gfm missing in X, Y, Z, and then show a pretty error for that” part but it is not good for other arbitrary errors (“show an (aggregated) error for the arbitrary non-missing errors we get.”).

In the case of vscode-remark, I imagine it’s useful to show the error message to the user, and log the full AggregateError including all stack traces in the logs for troubleshooting purposes.

I really doubt that those 4 error stacks help that user. I really think the only good fix for that user is in https://github.com/unifiedjs/unified-language-server/blob/73aa2bd151228ca5971befe4c8d76746e782e95c/lib/index.js#L198-L201.

remcohaszing commented 5 months ago

There are many possible errors, not just errors for unfound packages. Your pseudocode is a fine idea for the “So, we could detect if all errors are about gfm/remark-gfm missing in X, Y, Z, and then show a pretty error for that” part but it is not good for other arbitrary errors (“show an (aggregated) error for the arbitrary non-missing errors we get.”).

Why is it not good for arbritrary errors? My example error message stated it failed to load gfm/gfm-remark in X, Y, Z. That seems correct to me. It doesn’t say they are missing.

In the case of vscode-remark, I imagine it’s useful to show the error message to the user, and log the full AggregateError including all stack traces in the logs for troubleshooting purposes.

I really doubt that those 4 error stacks help that user. I really think the only good fix for that user is in https://github.com/unifiedjs/unified-language-server/blob/73aa2bd151228ca5971befe4c8d76746e782e95c/lib/index.js#L198-L201.

I agree those stack traces are probably not helpful for the user, only overwhelming even. However, those stack traces are useful when troubleshooting. If they are in the logs, the user is not bothered with them, but we can ask them to look it up if we need the extra information.

I’m currently investigating https://github.com/mdx-js/mdx-analyzer/issues/442. There appears to be some lower level issue. The only way to investigate this is to use the debugger to inspect the errors that are currently being swallowed by load-plugin.

wooorm commented 5 months ago

I’d love for you to try to solve this issue here inside your node_modules for 442, in 2 ways: one with just an aggregate error, and another with an improved error message that shows which paths were checked. That way, we can actually compare the different approaches.

Why is it not good for arbritrary errors? My example error message stated it failed to load gfm/gfm-remark in X, Y, Z. That seems correct to me. It doesn’t say they are missing.

These are talking about errors because something was not found in a particular path (the x, y, z). There are also many other reasons for errors to occur with import-meta-resolve, and then actually importing files. I’d imagine there are >=100 different errors possible.

remcohaszing commented 5 months ago

I resolved the issue. These are basically

After this I narrowed it down by debugging and tweaking a small Node.js script. It turns out import-meta-resolve uses URLs relative to the input. So both the local URL provided by mdx-analyzer and the global URL provided by load-plugin are wrong. The URLs should end with /. Trying to resolve ./node_modules/remark-frontmatter/package.json, at some point the URL constructor will strip mdx-ext-debug from file:///home/remco/Projects/mdx-ext-debug and resolve the URL to file:///home/remco/Projects/node_modules/remark-frontmatter/package.json. Likewise, at some point the URL constructor will strip node_modules from file:///home/remco/.local/lib/node_modules and resolve the URL to file:///home/remco/.local/lib/node_modules/remark-frontmatter/package.json. So resolving globally kind of works by accident.

In the end the fix was to add a trailing slash to the URL passed into load-plugin. (0e2e0e7f373eda12a38af0a7ab3b3f816efffdd4)

wooorm commented 5 months ago

Thank you!

Let’s use an patch load-plugin to use an AggregateError and a dedicated error message as per

Which patch exactly?

From just the error message I can see: […]

So what I am wondering is whether all that can be inferred by an improved error here. Have you checked that? Would you be interested in investigating that?

Neither of those is checked. (Which is fine IMO.)

IIRC you don‘t use nvm?

Now this is useful.

I don’t think the user would have been helped by showing them this aggregated error. In unified-engine I wrap errors from load-plugin. Have you considered wrapping errors from load-plugin in mdx-analyzer?

It turns out import-meta-resolve uses URLs relative to the input

Indeed, import.meta.resolve (and window.location and URL) works from files. The old CJS require.resolve works from folders

In the end the fix was to add a trailing slash to the URL passed into load-plugin.

That cwd variable seems to refer to path.dirname(tsconfig), perhaps instead pass pathToFileURL(tsconfig)?