-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle plugin errors #453
Handle plugin errors #453
Conversation
🦋 Changeset detectedLatest commit: 867becc The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
||
import FileAccess from '../filesystems/FileAccess.js'; | ||
import MutableVolume from '../filesystems/MutableVolume.js'; | ||
import createConfig from '../helpers/createConfig.js'; | ||
import { bindPluginMethods, bindSerialiser } from '../plugin/index.js'; | ||
import createSourceObservable from './helpers/createSourceObservable.js'; | ||
|
||
const trackPluginErrors = (errors: PluginErrors, lifecycleMethod: string) => { | ||
const errorsBuffer = Buffer.from(JSON.stringify({ errors, lifecycleMethod })); | ||
parentPort.postMessage({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugin methods that start with a $
run inside a child process and must communicate with the parent thread by posting a message.
packages/core/src/Source.ts
Outdated
@@ -36,6 +37,7 @@ export default class Source { | |||
#ignorePages: string[]; | |||
#workflows: SourceWorkflow[] = []; | |||
#schedule: SourceSchedule; | |||
#pluginErrors: any = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update the type of this when we have identified what we want to do with the errors. They are logged to the console but it would be nice to provide an API that can be used to see the errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth leaving this till after we merged in Next 13 changes... as right now, it's easy to rebase that branch, when we confine other changes to core and not change any of the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine...just pushed a change that will allow you to see the plugin errors when using the list sources admin API. Should be good enough for us to diagnose problems.
Pull Request Test Coverage Report for Build 5951061657
💛 - Coveralls |
The primary change included here is that any exceptions raised by plugins during any part of the plugin lifecycle are converted to instances of
PluginError
and tracked by the source that is running the plugins.This means plugin errors do not cause the source to close.
Plugin authors should be encouraged to throw a
PluginError
. Should an error occur when processing a particular page, then the full path to the page can be included in the error descriptor.I have also removed the
saveContent
plugin API. This was replaced withworkflows
some time ago and should have been removed then.Todo
Now that a source has a collection of plugin errors....how are these surfaced?