Skip to content

refactor(ama-mfe-ng-utils): simplify resize directive #3051

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −124
Open

refactor(ama-mfe-ng-utils): simplify resize directive #3051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@ama-mfe/ng-utils/src/resize/resize.consumer.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe('ResizeConsumerService', () => {
}
};
resizeHandlerService.supportedVersions['1.0'](resizeMessage);
expect(resizeHandlerService.newHeightFromChannel()).toEqual({ height: 500, channelId: 'test' });
expect(resizeHandlerService.heightPx()).toEqual(500);
});

it('should have the correct message type', () => {
Expand Down
10 changes: 4 additions & 6 deletions packages/@ama-mfe/ng-utils/src/resize/resize.consumer.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,14 @@ import {
/**
* This service listens for resize messages and updates the height of elements based on the received messages.
*/
@Injectable({
providedIn: 'root'
})
@Injectable()
export class ResizeConsumerService implements MessageConsumer<ResizeMessage> {
private readonly newHeight = signal<{ height: number; channelId: string } | undefined>(undefined);
private readonly heightPxSignal = signal<number | undefined>(undefined);

/**
* A readonly signal that provides the new height information from the channel.
*/
public readonly newHeightFromChannel = this.newHeight.asReadonly();
public readonly heightPx = this.heightPxSignal.asReadonly();

/**
* The type of messages this service handles ('resize').
Expand All @@ -46,7 +44,7 @@ export class ResizeConsumerService implements MessageConsumer<ResizeMessage> {
* Use the message paylod to compute a new height and emit it via the public signal
* @param message message to consume
*/
'1.0': (message: RoutedMessage<ResizeV1_0>) => this.newHeight.set({ height: message.payload.height, channelId: message.from })
'1.0': (message: RoutedMessage<ResizeV1_0>) => this.heightPxSignal.set(message.payload.height)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand well, with this change we will then limit the application to a single scalable iframe per app without strange behavior.
Not sure we want to bring this limitation at this stage to simplify the code.

Copy link
Member Author

@maxokorokov maxokorokov Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hey, yes, of course one day we might need a mechanism to support multiple apps visible at the same time (ex. resize and subroute navigation). It's just today's consumer/producer model doesn't support this, because they're not configurable and register via DI.

I'd say passing and appId as a part of a data model every time is not the best way to do it ;) ex. if we have other app-specific messages?

We need something like, even though it is not probably the best way to do it:

consumer.configure({
  from: ['appId']
})

// or make `.start()` configurable:
consumer.start({
  from: ['appId']
})

I'd also argue that it is not up to ResizeDirectve to configure the consumer, but to one who knows the appId (iframe level?). I can go further and argue that ResizeDirective shouldn't even exist just to do [style.height] binding, but it should be done by the iframe wrapper (but I won't ;).

I can add configuration in a separate PR, or later if necessary, I just don't think resize directive should even know about appId.

WDYT?

Copy link
Member Author

@maxokorokov maxokorokov Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Here, I made a picture ;)

Basically the same way as switching by message type at the consumer manager level, we should also allow consumer instances to filter who the message is from

Screenshot 2025-04-03 at 08 52 12

Maybe just adding from to the BasicConsumer would do the job:

export interface BasicMessageConsumer<T extends Message = Message> {
  type: T['type'];
  from: string[];
  supportedVersions: Record<string, MessageCallback<any>>;
}

or at the lower level (.start() or .configure()):

export interface MessageConsumer<T extends Message> extends BasicMessageConsumer {

  type: T['type'];
  supportedVersions: {
    [V in T['version']]: MessageCallback<T & { version: V }>;
  };

  configure(config);
  start(config): void;

  stop(): void;
}

};

private readonly consumerManagerService = inject(ConsumerManagerService);
Expand Down
90 changes: 18 additions & 72 deletions packages/@ama-mfe/ng-utils/src/resize/resize.directive.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
Component,
DebugElement,
Renderer2,
} from '@angular/core';
import {
ComponentFixture,
Expand All @@ -19,103 +18,50 @@ import {

@Component({
imports: [ScalableDirective],
standalone: true,
template: `<div [connect]="connect" [scalable]="scalableValue"></div>`
template: `<div scalable></div>`
})
class TestComponent {
public connect = 'testConnectId';
public scalableValue: string | undefined = 'testScalableId';
}
class TestComponent {}

describe('ScalableDirective', () => {
let parentComponentFixture: ComponentFixture<TestComponent>;
let directiveEl: DebugElement;
let directiveInstance: ScalableDirective;
let resizeHandlerService: ResizeConsumerService;
let renderer: Renderer2;

beforeEach(() => {
const resizeHandlerServiceMock = {
start: jest.fn(),
newHeightFromChannel: jest.fn().mockReturnValue({ channelId: 'testScalableId', height: 200 })
const resizeConsumerMock = {
heightPx: jest.fn().mockReturnValue(200)
};

parentComponentFixture = TestBed.configureTestingModule({
imports: [TestComponent, ScalableDirective],
providers: [
Renderer2,
{ provide: ResizeConsumerService, useValue: resizeHandlerServiceMock }

]
}).createComponent(TestComponent);
imports: [TestComponent, ScalableDirective]
})
.overrideDirective(ScalableDirective, {
set: {
providers: [{ provide: ResizeConsumerService, useValue: resizeConsumerMock }]
}
})
.createComponent(TestComponent);

parentComponentFixture.detectChanges();
directiveEl = parentComponentFixture.debugElement.query(By.directive(ScalableDirective));
directiveInstance = directiveEl.injector.get(ScalableDirective);
resizeHandlerService = directiveEl.injector.get(ResizeConsumerService);
renderer = directiveEl.injector.get(Renderer2);
});

it('should create an instance', () => {
expect(directiveInstance).toBeTruthy();
});

it('should start the resize handler service on initialization', () => {
expect(resizeHandlerService.start).toHaveBeenCalled();
});

it('should set the height style on the element with the channelId from scalable input', () => {
const channelId = 'scalable-channel-id';
jest.spyOn(resizeHandlerService, 'newHeightFromChannel').mockReturnValue({ height: 300, channelId });
const rendererSpy = jest.spyOn(renderer, 'setStyle');
parentComponentFixture.componentInstance.scalableValue = channelId;
parentComponentFixture.detectChanges();
expect(rendererSpy).toHaveBeenCalledWith(directiveEl.nativeElement, 'height', '300px');
rendererSpy.mockClear();
});

it('should set the height style on the element with the channelId from connect input', () => {
const channelId = 'connect-channel-id';
jest.spyOn(resizeHandlerService, 'newHeightFromChannel').mockReturnValue({ height: 400, channelId });
const rendererSpy = jest.spyOn(renderer, 'setStyle');
parentComponentFixture.componentInstance.scalableValue = undefined;
parentComponentFixture.componentInstance.connect = channelId;
parentComponentFixture.detectChanges();
expect(rendererSpy).toHaveBeenCalledWith(directiveEl.nativeElement, 'height', '400px');
rendererSpy.mockClear();
});

it('scalable input should take precedence over connect input', () => {
const connectChannelId = 'connect-channel-id';
const scalableChannelId = 'scalable-channel-id';
jest.spyOn(resizeHandlerService, 'newHeightFromChannel').mockReturnValue({ height: 400, channelId: scalableChannelId });
const rendererSpy = jest.spyOn(renderer, 'setStyle');
parentComponentFixture.componentInstance.scalableValue = scalableChannelId;
parentComponentFixture.componentInstance.connect = connectChannelId;
parentComponentFixture.detectChanges();
expect(rendererSpy).toHaveBeenCalledWith(directiveEl.nativeElement, 'height', '400px');
rendererSpy.mockClear();
});

it('should not update the style if channelId does not match', () => {
const connectChannelId = 'connect-channel-id';
const scalableChannelId = 'scalable-channel-id';
jest.spyOn(resizeHandlerService, 'newHeightFromChannel').mockReturnValue({ height: 400, channelId: scalableChannelId });
const rendererSpy = jest.spyOn(renderer, 'setStyle');
parentComponentFixture.componentInstance.scalableValue = 'not-matching-channel-id';
parentComponentFixture.componentInstance.connect = connectChannelId;
it('should set the height style on the element', () => {
jest.spyOn(resizeHandlerService, 'heightPx').mockReturnValue(300);
parentComponentFixture.detectChanges();
expect(rendererSpy).not.toHaveBeenCalled();
rendererSpy.mockClear();
expect(directiveEl.nativeElement.getAttribute('style')).toBe('height: 300px;');
});

it('should not set the height style on the element when newHeightFromChannel is not available', () => {
const channelId = 'scalable-channel-id';
jest.spyOn(resizeHandlerService, 'newHeightFromChannel').mockReturnValue(undefined);
const rendererSpy = jest.spyOn(renderer, 'setStyle');
parentComponentFixture.componentInstance.scalableValue = channelId;
it('should not set the height style on the element when heightPx is not available', () => {
jest.spyOn(resizeHandlerService, 'heightPx').mockReturnValue(undefined);
parentComponentFixture.detectChanges();
expect(rendererSpy).not.toHaveBeenCalled();
rendererSpy.mockClear();
expect(directiveEl.nativeElement.getAttribute('style')).toBe('');
});
});
50 changes: 5 additions & 45 deletions packages/@ama-mfe/ng-utils/src/resize/resize.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import {
computed,
Directive,
effect,
ElementRef,
inject,
input,
Renderer2,
} from '@angular/core';
import {
ResizeConsumerService,
Expand All @@ -16,46 +11,11 @@ import {
*/
@Directive({
selector: '[scalable]',
standalone: true
host: {
'[style.height.px]': 'resizeConsumer.heightPx()'
},
providers: [ResizeConsumerService]
})
export class ScalableDirective {
/**
* The connection ID for the element, used as channel id backup
*/
public connect = input<string>();

/**
* The channel id
*/
public scalable = input<string>();

private readonly resizeHandler = inject(ResizeConsumerService);

/**
* This signal checks if the current channel requesting the resize matches the channel ID from the resize handler.
* If they match, it returns the new height information; otherwise, it returns undefined.
*/
private readonly newHeightFromChannel = computed(() => {
const channelAskingResize = this.scalable() || this.connect();
const newHeightFromChannel = this.resizeHandler.newHeightFromChannel();
if (channelAskingResize && newHeightFromChannel?.channelId === channelAskingResize) {
return newHeightFromChannel;
}
return undefined;
});

constructor() {
const elem = inject(ElementRef);
const renderer = inject(Renderer2);

this.resizeHandler.start();

/** When a new height value is received set the height of the host element (in pixels) */
effect(() => {
const newHeightFromChannel = this.newHeightFromChannel();
if (newHeightFromChannel) {
renderer.setStyle(elem.nativeElement, 'height', `${newHeightFromChannel.height}px`);
}
});
}
protected readonly resizeConsumer = inject(ResizeConsumerService);
}
Loading