-
Notifications
You must be signed in to change notification settings - Fork 22
feat: angular sdk overhaul #132
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
base: master
Are you sure you want to change the base?
feat: angular sdk overhaul #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR represents a major overhaul of the ImageKit Angular SDK, upgrading it from version 5.x to 6.x. The changes include a package rename, API simplification, architectural improvements, and enhanced testing infrastructure.
Key Changes:
- Package renamed from
imagekitio-angularto@imagekit/angularwith updated module structure - API simplified by removing
pathparameter (replaced withsrc), removingpublicKeyfrom configuration, and removing LQIP feature - Upload component removed in favor of functional
upload()API re-exported from core JavaScript SDK - New responsive image support enabled by default with automatic srcset generation
Reviewed changes
Copilot reviewed 69 out of 84 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/package.json | Updated package name, version to 6.0.0, dependencies, and build configuration |
| sdk/src/lib/imagekit-angular.module.ts | Renamed module from ImagekitioAngularModule to ImagekitAngularModule, removed upload component |
| sdk/src/lib/imagekit-angular.service.ts | Simplified service to use @imagekit/javascript buildSrc instead of imagekit-javascript SDK |
| sdk/src/lib/ik-image/ik-image.component.ts | Added responsive image support with srcset generation, removed LQIP functionality |
| sdk/src/lib/ik-video/ik-video.component.ts | Replaced path parameter with src, simplified component logic |
| sdk/src/public-api.ts | Updated exports to re-export functions and types from @imagekit/javascript |
| test-app/ | New comprehensive test application demonstrating SDK features with Cypress E2E tests |
| README.md | Extensive documentation updates including migration guide, new upload examples, and API changes |
| sdk/tests/sdk-tests/ | Updated unit tests to reflect API changes and removed upload component tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review for regressions as part of the overhaul |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 68 out of 83 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| template: `<video></video>`, | ||
| }) | ||
|
|
||
| export class IkVideoComponent implements OnInit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should also implement OnChanges and AfterViewInit since we have the functions ngAfterViewInit() and ngOnChanges().
Ref:
| export class IkVideoComponent implements OnInit { | |
| export class IkVideoComponent implements OnInit, OnChanges, AfterViewInit { |
| @Input('queryParameters') queryParameters: QueryParameters; | ||
| @Input('loading') loading: string = "lazy"; | ||
| @Input('width') width: number | string; | ||
| @Input('height') height: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If width is number | string, shouldn't this be the same?
| @Input('height') height: string; | |
| @Input('height') height: number | string; |
| this.setElementAttributes(video, attrsToSet); | ||
| } | ||
|
|
||
| namedNodeMapToObject(source: NamedNodeMap): Dict { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same function in ik-image component too. Let's extract this to a shared util.
| context.setElementAttributes(image, attrsToSet); | ||
| } | ||
|
|
||
| namedNodeMapToObject(source: NamedNodeMap): Dict { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same function in ik-video component too. Let's extract this to a shared util.
| return target; | ||
| }; | ||
|
|
||
| setElementAttributes(element: any, attributesLiteral: Dict): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same function in ik-video component too. Let's extract this to a shared util.
| this.setUrl(options); | ||
| } | ||
|
|
||
| ngOnChanges(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling lifecycle hooks manually is anti-pattern. Instead, extract the logic in these hooks into separate functions and then call them from both the places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye aye
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this
| }; | ||
| this.setUrl(options); | ||
| } | ||
| ngOnChanges(): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling lifecycle hooks manually is anti-pattern. Instead, extract the logic in these hooks into separate functions and then call them from both the places.
| this.url = buildSrc(strictOptions); | ||
| this.srcset = ''; | ||
| } else { | ||
| const { src: newSrc, srcSet } = getResponsiveImageAttributes({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this function errors out? We should handle it in try-catch.
| const strictOptions = options as SrcOptions; | ||
|
|
||
| if (!this.responsive) { | ||
| this.url = buildSrc(strictOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this function errors out? We should handle it in try-catch.
| return null; | ||
| } | ||
| const strictOptions = options as SrcOptions | ||
| this.url = buildSrc(strictOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this function errors out? We should handle it in try-catch.
No description provided.