Provide generics for Image class #254
Open
+79
−78
Conversation
This file contains hidden or 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 = {}> = { |
Contributor
There was a problem hiding this comment.
why this type is not reused below?
Contributor
Author
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>; |
Contributor
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
|
|
||
| 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; |
Contributor
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> |
Contributor
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
| type ImageToolConstructorOptions< CustomActions = {},AdditionalUploadResponse = {}> = BlockToolConstructorOptions<ImageToolData<CustomActions, AdditionalUploadResponse>, ImageConfig> | ||
|
|
||
| export default class ImageTool implements BlockTool { | ||
| export default class ImageTool<CustomActions = {} ,AdditionalUploadResponse = {}> implements BlockTool { |
Contributor
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
| * @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>> { |
Contributor
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
2 participants
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.