chore: migrate plugin types to plugin #2111
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A significant change to how we type plugins.
Type Declaration
Instead of declaring all plugin types in
plugins/plugins-types.d.ts
, we declare each plugins types in the respective plugin using a JSDoc@typedef
. Then we import that typedef inplugins/plugins-types.d.ts
.This keeps the types much closer to the implementation, and means when amending parameters we only need to touch one file instead of remembering to update the separate type declarations file.
I opted for using the more verbose
@typedef
syntax (@property {type} name
) over the leaner variant ({{ name: type }}
) because I prefer that the verbose syntax leaves room to document types as well. You'll see some types even had their documentation migrated over to the JSDoc as well.Refactors
InternalParams
forcovertPathData
. Instead, we use theRequired<T>
type which sets all types to non-null without us having to do it ourselves.Plugin
type fromplugins/plugins-types.d.ts
, in favor of using the type with the same name fromlib/types.d.ts
. With the new way we're defining types now, there is no value in keeping that one, and as we're prepping for SVGO v4, we can drop it from the public API.Bugs
removeXlink
, which had the parameter defined as required when it should've been optional.Documentation