Provide generics for Image class #254
Open
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
neSpecc
reviewed
Jul 9, 2024
src/types/types.ts
Outdated
| /** | ||
| * Represents the format of a file object data with the additional data. | ||
| */ | ||
| export type FileObjectData<AdditionalData = {}> = { |
There was a problem hiding this comment.
why this type is not reused below?
There was a problem hiding this comment.
it was initially defined to be used to defined parameters types of member functions. But it can be used in the interfaces if you would like to.
neSpecc
reviewed
Jul 13, 2024
src/index.ts
Outdated
|
|
||
| /** | ||
| * UI module instance | ||
| */ | ||
| private ui: Ui; | ||
| private ui: Ui<CustomActions, AdditionalUploadResponse>; |
There was a problem hiding this comment.
I think it would be better to pass ImageToolData<CustomActions, AdditionalUploadResponse> because UI should not depend on AdditionalUploadResponse, it depends on data format instead since it may be used for rendering
neSpecc
reviewed
Jul 13, 2024
src/index.ts
Outdated
| this.image = data.file; | ||
|
|
||
| this._data.caption = data.caption || ''; | ||
| this.ui.fillCaption(this._data.caption); | ||
|
|
||
| ImageTool.tunes.forEach(({ name: tune }) => { | ||
| const value = typeof data[tune as keyof ImageToolData] !== 'undefined' ? data[tune as keyof ImageToolData] === true || data[tune as keyof ImageToolData] === 'true' : false; | ||
| const value = typeof data[tune as keyof ImageToolData<CustomActions, AdditionalUploadResponse>] !== 'undefined' ? data[tune as keyof ImageToolData<CustomActions, AdditionalUploadResponse>] === true || data[tune as keyof ImageToolData<CustomActions, AdditionalUploadResponse>] === 'true' : false; |
There was a problem hiding this comment.
this line is too long and hard to read. Can we do something with it?
neSpecc
reviewed
Jul 13, 2024
src/index.ts
Outdated
|
|
||
|
|
||
| type ImageToolConstructorOptions = BlockToolConstructorOptions<ImageToolData, ImageConfig> | ||
| type ImageToolConstructorOptions< CustomActions = {},AdditionalUploadResponse = {}> = BlockToolConstructorOptions<ImageToolData<CustomActions, AdditionalUploadResponse>, ImageConfig> |
There was a problem hiding this comment.
Suggested change
| type ImageToolConstructorOptions< CustomActions = {},AdditionalUploadResponse = {}> = BlockToolConstructorOptions<ImageToolData<CustomActions, AdditionalUploadResponse>, ImageConfig> | |
| type ImageToolConstructorOptions<CustomActions = {}, AdditionalUploadResponse = {}> = BlockToolConstructorOptions<ImageToolData<CustomActions, AdditionalUploadResponse>, ImageConfig> |
src/index.ts
Outdated
|
|
||
| export default class ImageTool implements BlockTool { | ||
| export default class ImageTool<CustomActions = {} ,AdditionalUploadResponse = {}> implements BlockTool { |
There was a problem hiding this comment.
Suggested change
| export default class ImageTool<CustomActions = {} ,AdditionalUploadResponse = {}> implements BlockTool { | |
| export default class ImageTool<CustomActions = {}, AdditionalUploadResponse = {}> implements BlockTool { |
neSpecc
reviewed
Aug 3, 2024
src/utils/isPromise.ts
Outdated
| @@ -5,6 +5,6 @@ import type { UploadResponseFormat } from '../types/types'; | |||
| * @param object - object to check | |||
| * @returns | |||
| */ | |||
| export default function isPromise(object: Promise<UploadResponseFormat>): object is Promise<UploadResponseFormat> { | |||
| export default function isPromise<AdditionalUploadResponse>(object: Promise<UploadResponseFormat<AdditionalUploadResponse>>): object is Promise<UploadResponseFormat<AdditionalUploadResponse>> { | |||
There was a problem hiding this comment.
isPromise is a util, so it should not depend on UploadResponseFormat and AdditionalUploadResponse.
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.
Problem
The current design of the
Imageclass doesn't make it easily extendable so that users can define their actions and additional fields in the upload response. This also limits class extensions for use cases where custom data and actions must be implemented.Cause
The
Imageclass was first designed with types fixed for the actions and upload response data, which is a very concrete design, and it limited the flexibility with different kinds of requirements. The approach taken in designing thus becomes relatively narrow and does not allow an extension of the user's custom properties during the creation ofImageobject.Solution
Generic types should be used to re-implement the
Imageclass to fix this problem. The definition of the class should change into classImageTool<CustomActions = {}, AdditionalUploadResponse = {}>, such that while the object of it is created, users can provide their custom actions and additional upload response types. This makes theImageclass flexible and more usable and leaves the ability to respond to almost any scenario and requirements.