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.

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

Conversation

maxokorokov
Copy link
Member

Proposed change

This is for discussion first, I haven't modified tests yet.

Simplify resize directive and message consumer. Three changes:

  1. Using [style.height.px] simplifies things greatly
  2. connectId is handled at the consumer level, not at the directive level. Also I don't see the point in providing it in root as it is tightly coupled with connectId and directive instance.
  3. I'm not sure I understand why there are both [connect] and [scalable] inputs? Can we just remove scalable input and use connect? I don't really see the point in specifying BOTH.

WDYT?

@maxokorokov maxokorokov requested a review from a team as a code owner March 25, 2025 16:21
Copy link

nx-cloud bot commented Mar 25, 2025

View your CI Pipeline Execution ↗ for commit 6b0457e.

Command Status Duration Result
nx run-many --target=test-e2e ✅ Succeeded 3m 20s View ↗
nx run-many --target=test-int ✅ Succeeded 1s View ↗
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=publish --nx-bail --userco... ✅ Succeeded 31s View ↗
nx affected --target=test --collectCoverage --c... ✅ Succeeded 35s View ↗
nx run-many --target=prepare-publish --exclude-... ✅ Succeeded 7s View ↗
nx run-many --target=documentation ✅ Succeeded 1m 26s View ↗
nx run ama-sdk-schematics:build-swagger ✅ Succeeded 4s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-04-02 22:19:02 UTC

@matthieu-crouzet
Copy link
Contributor

LGTM

@mrednic-1A
Copy link
Contributor

nice work :) it simplifies a lot the things. ok for points 1 and 2.
For 3 still ok, and as you changed the selector of the directive there is no need of scalable input :)

Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.60%. Comparing base (9a333ef) to head (6b0457e).

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- remove unnecessary inputs
- remove the notion of `channelId`
- simplify style binding
@maxokorokov
Copy link
Member Author

@mrednic-1A, @kpanot, @matthieu-crouzet please take another look.

We've decided with @mrednic-1A to simplify it even more and remove completely the notion of 'who was resize message sent from'. We might introduce it later if necessary at the higher ConsumerManager level.

@maxokorokov maxokorokov requested a review from kpanot April 2, 2025 22:12
@@ -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

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

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;
}

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