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] Review types feedback #13864

Closed
Witoso opened this issue Apr 12, 2023 · 7 comments
Closed

[TS] Review types feedback #13864

Witoso opened this issue Apr 12, 2023 · 7 comments
Labels
domain:ts resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@Witoso
Copy link
Member

Witoso commented Apr 12, 2023

Review CoreMedia's feedback about exports in #12027, and decide on the next steps.

#12027 (comment)

@Witoso Witoso added squad:core Issue to be handled by the Core team. domain:ts type:improvement This issue reports a possible enhancement of an existing feature. labels Apr 12, 2023
@Reinmar
Copy link
Member

Reinmar commented Apr 12, 2023

Those are some interesting examples of missing exports. I feel we may need to discuss it because some of these classes are not something we would like to expose in order to ensure flexibility for us to maintain (e.g. refactor) those features. But I understand the need to export them.

@mmichaelis
Copy link

Those are some interesting examples of missing exports. I feel we may need to discuss it because some of these classes are not something we would like to expose in order to ensure flexibility for us to maintain (e.g. refactor) those features. But I understand the need to export them.

Some are related to yet missing extension points, like for the link UI (see #13446). That's why I added a reference to our usages, as I understand that it is not desirable to expose all layers of the API. Thus, I like the idea of having index.ts as clear "public API" contract.

@Witoso
Copy link
Member Author

Witoso commented Apr 12, 2023

@mmichaelis, we had a chance to discuss it today:

  • as for the image, looks like an oversight from our side, we will add proper exports to all image plugins. Trackable here: [TS] Fix exports in the Image package #13868
  • as for the imports that are done just by tests, please continue doing it the way you do. In the cases, you mentioned we haven't seen a valid case of exposing it in the public API.
  • in other cases (used in the production code), it would be great if you could describe in a few words the context of usage. It will be easier for us to decide if it should be placed on an index level.

@Witoso Witoso added the pending:feedback This issue is blocked by necessary feedback. label Apr 17, 2023
@mmichaelis
Copy link

in other cases (used in the production code), it would be great if you could describe in a few words the context of usage. It will be easier for us to decide if it should be placed on an index level.

Some details with focus on LinkActionsView and LinkFormView:

Our use-cases are:

  • Add action buttons to LinkActionsView (here: to set target attribute).
  • Modify LinkActionsView and LinkFormView to not only hold plain text, but possibly also references to CMS contents (which feels really nice, having it integrated here).

Especially for the latter case, we require deep integration into the views, also adding observable properties (via set) to them.

Unfortunately, not exposing LinkActionsView and LinkFormView in index.ts and/or not having them as named exports shipped by CKEditor 5, makes augmentation scenarios close to impossible, because they struggle with:

Currently trying the suggested [workaround|https://github.com/microsoft/TypeScript/issues/14080#issuecomment-1050833256] (instead to give up with @ts-expect-error), but it would be much easier, having them provided by CKEditor 5 API as named exports.

Thus, this may be two in one:

@Witoso Witoso removed the pending:feedback This issue is blocked by necessary feedback. label Apr 24, 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
@Witoso
Copy link
Member Author

Witoso commented Apr 26, 2023

thanks again for the feedback @mmichaelis. We are discussing this internally as the UI's API is a tricky subject, and we want to make sure we are exposing proper things. Glad there's a workaround, we will get back to you as soon as we know something.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels May 27, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ts resolution:expired This issue was closed due to lack of feedback. 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

No branches or pull requests

4 participants