Skip to content
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

[TS] Support Optional Properties in ObservableMixin (here: SingleBindChain) #13965

Closed
mmichaelis opened this issue Apr 25, 2023 · 1 comment · Fixed by #14110
Closed

[TS] Support Optional Properties in ObservableMixin (here: SingleBindChain) #13965

mmichaelis opened this issue Apr 25, 2023 · 1 comment · Fixed by #14110
Assignees
Labels
domain:ts squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@mmichaelis
Copy link

📝 Provide a description of the improvement

While developing our own plugins for CKEditor 5, we would prefer the ObservableMixin (here: SingleBindChain) to support binding from/to optional values as well.

Reproducer

The following will report an error in LinkUI if we define the command value (here LinkCommand, also requires change in Command) to be optional:

actionsView.bind( 'href' ).to( linkCommand, 'value' );

To reproduce, just change these definitions to optional rather than enforcing either a value or undefined:

declare public value: string | undefined;

and

declare public value: unknown;

Thus, for the given lines:

// linkcommand.ts
declare public value?: string;
// command.ts
declare public value?: unknown; 

(side note: As not all commands require having a value, optional might even be the better choice here)

Suggestion

The suggestion is changing, for example (related to the example above):

to<O extends Observable & { [ P in K ]: TVal }, K extends keyof O>(
observable: O,
key: K
): void;

to:

to<O extends Observable & { [ P in K ]?: TVal }, K extends keyof O>(
	observable: O,
	key: K
): void;

Most likely, other declaration should be adapted, too.

Workaround

Despite using @ts-expect-error (in favor of @ts-ignore), you have to define your bound properties as not being optional but either requiring a value or undefined just as it is done for LinkCommand.

As an alternative to this, redefine the given method via Module Augmentation, as also used in augmentation.ts:

declare module "@ckeditor/ckeditor5-utils" {
  interface SingleBindChain<TKey extends string, TVal> {
    to<O extends Observable & { [ P in K ]?: TVal }, K extends keyof O>(
      observable: O,
      key: K
    ): void
  }
}

This, seems to require splitting up bind and to, though, to (using the LinkUI example from above):

const bindChain = actionsView.bind( 'href' );
bindChain.to( linkCommand, 'value' );

📃 Other details

  • Browser: Chrome
  • OS: Windows
  • CKEditor version: 37.0.1
  • Installed CKEditor plugins: not relevant

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@mmichaelis mmichaelis added the type:improvement This issue reports a possible enhancement of an existing feature. label Apr 25, 2023
@Witoso Witoso added squad:core Issue to be handled by the Core team. domain:ts labels Apr 25, 2023
mmichaelis added a commit to CoreMedia/ckeditor-plugins that referenced this issue Apr 25, 2023
Includes augmenting `LinkUI` by new `LinkActionsView` and
`LinkFormView` supporting our (again augmented) properties
`contentUriPath` and `contentName`. Due to several issues
regarding not providing `LinkFormView` and `LinkActionsView`
as named exports (which blocks augmenting them due to an
unresolved TypeScript Issue), and `SingleBindChain.to`
having too strict typing - at least from the implementation
point of view here, this resulted in a bunch of changes
to get typings straight.

See-also: microsoft/TypeScript#14080
See-also: ckeditor/ckeditor5#13864
See-also: ckeditor/ckeditor5#13965
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Apr 26, 2023
@Witoso
Copy link
Member

Witoso commented Apr 26, 2023

Thanks for the feedback, we will take a look at this.

@arkflpc arkflpc self-assigned this May 2, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels May 2, 2023
arkflpc added a commit that referenced this issue May 15, 2023
Feature (utils). Add support for optional properties in `ObservableMixin` bind chain. Closes #13965.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label May 15, 2023
@CKEditorBot CKEditorBot added this to the iteration 64 milestone May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ts squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants