Skip to content

Conversation

@SwarnimDoegar
Copy link

No description provided.

@ahnv ahnv requested a review from Copilot December 17, 2025 05:20
Copy link

Copilot AI left a 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-angular to @imagekit/angular with updated module structure
  • API simplified by removing path parameter (replaced with src), removing publicKey from 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.

@ahnv
Copy link
Member

ahnv commented Dec 17, 2025

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

@ahnv
Copy link
Member

ahnv commented Dec 17, 2025

@codex review for regressions as part of the overhaul

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

Copy link

Copilot AI left a 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 {
Copy link
Member

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:

  1. https://v17.angular.io/api/core/OnChanges
  2. https://v17.angular.io/api/core/AfterViewInit
Suggested change
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;
Copy link
Member

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?

Suggested change
@Input('height') height: string;
@Input('height') height: number | string;

this.setElementAttributes(video, attrsToSet);
}

namedNodeMapToObject(source: NamedNodeMap): Dict {
Copy link
Member

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 {
Copy link
Member

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 {
Copy link
Member

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 {
Copy link
Member

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.

Ref: https://www.linkedin.com/pulse/why-you-shouldnt-trigger-lifecycle-hooks-manually-prakhar-srivastava-ybxnf/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye aye

Copy link
Author

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 {
Copy link
Member

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.

Ref: https://www.linkedin.com/pulse/why-you-shouldnt-trigger-lifecycle-hooks-manually-prakhar-srivastava-ybxnf/

this.url = buildSrc(strictOptions);
this.srcset = '';
} else {
const { src: newSrc, srcSet } = getResponsiveImageAttributes({
Copy link
Member

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);
Copy link
Member

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);
Copy link
Member

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.

import { buildSrc, getResponsiveImageAttributes, SrcOptions } from '@imagekit/javascript';
import type { Transformation } from '@imagekit/javascript'

@Component({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Components are not marked as standalone, preventing direct import in standalone Angular applications (14+).

The SDK claims standalone support but only provides half the solution - it has the provider function but the components themselves aren't standalone. Consumers must still import the entire ImagekitAngularModule even in standalone apps, which defeats the purpose of Angular's modern standalone API.

## Version Support
| SDK Version | Angular 11.x | Angular 12.2.x | Angular 13.x | Angular 14.x | Angular 15.x | Angular 16.x | Angular 17.x |
|-------------|---------|---------|---------|---------|---------|---------|---------|
| 6.x || ✔️ | ✔️ | ✔️ | ✔️ | ✔️ |✔️ |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version support table ends at Angular 17, but Angular 18 is the current LTS (released May 2024, LTS until Nov 2025).


ImageKit is complete media storage, optimization, and transformation solution that comes with an image and video CDN. It can be integrated with your existing infrastructure - storage like AWS S3, web servers, your CDN, and custom domain names, allowing you to deliver optimized images in minutes with minimal code changes.

## Breaking changes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove from here. Follow other frontend SDKs (Next, React, Vue) for changelog format.

"ng-packagr": "^12.1.1",
"typescript": "~4.3.5"
}
"name": "@imagekit/angular",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just verify this once.

Missing modern exports field for optimal tree-shaking and subpath imports.

{
  "exports": {
    ".": {
      "types": "./index.d.ts",
      "esm2022": "./esm2022/imagekit-angular.mjs",
      "esm": "./esm/imagekit-angular.mjs",
      "default": "./fesm2022/imagekit-angular.mjs"
    }
  }
}

Note: ng-packagr v12+ should generate this automatically. Verify in built package.

"name": "@imagekit/angular",
"version": "6.0.0",
"description": "Angular SDK for ImageKit.io - Image and video optimization, transformation, and delivery platform",
"keywords": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we safely add sideEffects: false declaration for tree-shaking optimization, assuming SDK has no global side effects.

"peerDependencies": {
"@angular/common": ">=12.2.0",
"@angular/core": ">=12.2.0",
"@imagekit/javascript": "^5.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imagekit/javascript needs to be in dependencies. That's it. It is in peer and dev dependencies at the moment. Makes no sense. Check react pacakge.json https://github.com/imagekit-developer/imagekit-react/blob/master/package.json

}
}

loadImage(context: IkImageComponent, url: string, srcset: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSR Support is Missing

Problem:

This SDK does not support Server-Side Rendering (SSR) and will crash in Angular Universal applications.

Root Cause:

Components directly manipulate the DOM using browser-only APIs:

  • ElementRef.nativeElement access in lifecycle hooks
  • Direct DOM manipulation via nativeElement.attributes and nativeElement.children.

Example of the Issue:

// Current code in ik-image.component.ts (lines 88-109)
loadImage(context: IkImageComponent, url: string, srcset: string): void {
  const nativeElement = context.el.nativeElement;  // ❌ Crashes on server
  const attributes = nativeElement.attributes;      // ❌ Browser-only API
  const image = nativeElement.children[0];         // ❌ Browser-only API
  setElementAttributes(image, attrsToSet, context.selector);
}

Impact:

❌ Cannot use with Angular Universal (SSR)
❌ Cannot use with Angular prerendering
❌ Blocks adoption for SEO-critical applications
❌ React/Vue/Next SDKs all support SSR - we're behind

// For file uploads, see the File Upload section
```

## Demo application
Copy link
Contributor

@imagekitio imagekitio Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated readme. Moreover it needs to follow convention of React/JS/Vue/Next SDK repos. We point to our quick start docs instead of repeating ourselves here in readme.

URL_ENDPOINT: ${{ secrets.ik_url_endopint }}
e2e:
runs-on: ubuntu-latest
needs: build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Node.js 14.x which reached End-of-Life on April 30, 2023.

"baseUrl": "./",
"outDir": "./dist/out-tsc",
"forceConsistentCasingInFileNames": true,
"strict": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict mode disabled, reducing type safety guarantees. Why we did this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants