-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
types: preset and pluggable list settings type checking #92
Closed
ChristianMurphy
wants to merge
2
commits into
unifiedjs:main
from
ChristianMurphy:types/pluggable-list-checking
Closed
types: preset and pluggable list settings type checking #92
ChristianMurphy
wants to merge
2
commits into
unifiedjs:main
from
ChristianMurphy:types/pluggable-list-checking
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
when `use([plugin, setting])` syntax is used, typescript can now warn if the settings object does not match with what the plugin expects.
ChristianMurphy
added
🦋 type/enhancement
This is great to have
🧑 semver/major
This is a change
☂️ area/types
This affects typings
🌊 blocked/upstream
This cannot progress before something external happens first
labels
Jun 27, 2020
This comment has been minimized.
This comment has been minimized.
ChristianMurphy
commented
Jun 27, 2020
Pluggable<P, S18>?, | ||
Pluggable<P, S19>?, | ||
Pluggable<P, S20>?, | ||
...Array<Pluggable<P, 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.
allows more plugins to be added past 20, but without enhanced settings type checking
ChristianMurphy
force-pushed
the
types/pluggable-list-checking
branch
2 times, most recently
from
June 27, 2020 21:02
80456e5
to
3a2c185
Compare
leverages variadic typing and generics to allow the first 20 plugins to have full settings type checking. plugins after the first twenty can still be added, but no longer have their settings checked. BREAKING CHANGE: changes to order of the processor settings generic `P`, to go first and plugin settings `S` to go after. Plugins directly using the `Plugin` or `Attacher` type will need to add the first generic with the processor settings. NOTE: If the plugin does not directly use the `Plugin` or `Attacher` type it may not need to migrate. For example ```ts // this does NOT need to migrate, it will continue to work const plugin = () => (settings: Settings) => { /* ... */ } // this DOES need to migrate, settings would passed to the wrong generic parameter const plugin: Plugin<[Settings?]> = () => (settings: Settings) => { /* ... */ } ``` MIGRATION: if `this` is not used in the plugin, set the first generic to `unknown` For example ```typescript Plugin<[PluginSettings?]> // becomes Plugin<unknown, [PluginSettings?]> // and Attacher<[PluginSettings?]> // becomes Attacher<unknown, [PluginSettings?]> ``` if it does depend on `this`, add the processor options in the first generic, for example `RemarkOptions` or `RehypeOptions` could be used.
ChristianMurphy
force-pushed
the
types/pluggable-list-checking
branch
from
June 27, 2020 21:24
3a2c185
to
1bcbfe7
Compare
/cc @remcohaszing |
ChristianMurphy
added
☂️ area/types
This affects typings
and removed
☂️ area/types
This affects typings
🌊 blocked/upstream
This cannot progress before something external happens first
💬 type/discussion
This is a request for comments
🦋 type/enhancement
This is great to have
🧑 semver/major
This is a change
labels
Aug 21, 2020
1 task
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
builds on #91
allow pluggable list and preset to check settings on first 20 elements
leverages variadic typing and generics to allow the first 20 plugins to have full settings type checking.
plugins after the first twenty can still be added, but no longer have their settings checked.
BREAKING CHANGE: changes to order of the processor settings generic
P
, to go first and plugin settingsS
to go after. Plugins directly using thePlugin
orAttacher
type will need to add the first generic with the processor settings.NOTE: If the plugin does not directly use the
Plugin
orAttacher
type it may not need to migrate. For exampleMIGRATION: if
this
is not used in the plugin, set the first generic tounknown
For example
if it does depend on
this
, add the processor options in the first generic, for exampleRemarkOptions
orRehypeOptions
could be used.Additional Discussion:
PluggableList
? Pointers on the syntax for that would be welcome!Attacher
andPlugin
keep their current generic order? This would reduce the need for migration, but would introduce incosistency between generic parameter order forAttacher
andPlugin
vsPluggableList
andPreset