Inline task actions - strong types check#7851
Inline task actions - strong types check#7851ChristopherDedominici wants to merge 10 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 17f2db2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
ChristopherDedominici
added
the
no docs needed
| nextTaskArguments: TaskArguments, | ||
| currentIndex = this.actions.length - 1, | ||
| ): Promise<any> => { | ||
| const currentTaskAction = this.actions[currentIndex]; |
There was a problem hiding this comment.
This is where the run command decides whether to run the action or the inlineAction, depending on which one is defined.
alcuadrado
left a comment
alcuadrado
left a comment
There was a problem hiding this comment.
I left some comments.
I think the main one is weather if the type complexity is worth it or not.
Also, this PR will need to update the config validation. The reason is that a user can provide a task definition that wasn't created with the builders, so we need to validate them.
| /** | ||
| * A helper type to strictly enforce that plugins only use file-based actions. | ||
| */ | ||
| export type PluginSafeTaskDefinition = |
There was a problem hiding this comment.
Maybe let's call it just PluginTaskDefinition. The Safe part makes it a bit confusing.
| } | ||
|
|
||
| export type TaskAction = | ||
| | { action: LazyActionObject<NewTaskActionFunction>; inlineAction?: never } |
There was a problem hiding this comment.
What about renaming this to:
| | { action: LazyActionObject<NewTaskActionFunction>; inlineAction?: never } | |
| | { lazyAction: LazyActionObject<NewTaskActionFunction>; inlineAction?: never } |
this way any error message will be more clear.
There was a problem hiding this comment.
I agree, but this will break current plugins, is this ok?
There was a problem hiding this comment.
This will be a breaking change, so are we okay with that?
| | MissingActionState | ||
| | ActionDefinedState | ||
| | DuplicateActionError = MissingActionState, | ||
| ActionTypeT extends "FILE" | "INLINE" | "NONE" = "NONE", |
There was a problem hiding this comment.
what about something easier to read, like this? Again, because this may leak to type errors
| ActionTypeT extends "FILE" | "INLINE" | "NONE" = "NONE", | |
| ActionTypeT extends "LAZY_ACTION" | "INLINE_ACTION" | "MISSING_ACTION" = "MISSING_ACTION", |
| ActionTypeT extends "FILE" | "INLINE" | "NONE" = "NONE", | ||
| > { | ||
| /** | ||
| * Technical property needed to enforce the `ActionStateT` state check at compile time. |
There was a problem hiding this comment.
Is this really needed? What happens if you omit it? I think a common name, at least in rust, would be Phantom: https://doc.rust-lang.org/std/marker/struct.PhantomData.html
| TaskArgumentsT, | ||
| DuplicateActionError, | ||
| "FILE" | "INLINE" | ||
| >; |
There was a problem hiding this comment.
I understand this, but maybe it's too much? I think if the duplicated action doesn't fail at type-level, but fails immediately in the builder that's fine. It's a bug that you would catch in the very first run.
Same with missing action.
Removing them would simplify the code, at the expenses of only finding the error in the first run (I think that's fine), and adding some runtime validation in the builders and config validation.
Note that the validation is still needed, as the user may be using js.
There was a problem hiding this comment.
Would we loose anything else?
There was a problem hiding this comment.
This also applies for the other kind of task definitions
ChristopherDedominici
changed the title
|
@alcuadrado I created a separate PR where most of the checks have been modified to be runtime checks, see here (see here), so we can have a direct comparison between the two PRs. |
3 participants
Fixes #7560
Same as 7950, but here checks are mostly executed at type level