RevealBi / reveal-sdk-wrappers

Other
0 stars 1 forks source link

WebComponents Feedback #2

Open dwilches opened 4 weeks ago

dwilches commented 4 weeks ago

I'm testing the Beta of Web Components, and the documentation instructs me to use CUSTOM_ELEMENTS_SCHEMA. Using this is something we wouldn't like to do in our app as that makes Angular's compiler to not warn us about legitimate errors, like a typo in a component's name or a component that is outside of the scope of the current module.

Moreover, the documentation says:

Add this schema to your application's module

Which is not really accurate, it should be added to the module from where the users are going to invoke the Reveal web components. In Angular, each module can choose to use CUSTOM_ELEMENTS_SCHEMA, and only in those modules where you allow unknown elements you can use unknown elements (it is not inherited by submodules either).

Instead of instructing your users to use CUSTOM_ELEMENTS_SCHEMA and the defineRevealSdkWrappers() you could encapsulate both in an Angular module, hiding the fact we're using web components as they'll be plain Angular components. This can be done by creating a module called RevealWebComponentsModule, which invokes defineRevealSdkWrappers() and contains CUSTOM_ELEMENTS_SCHEMA. Then your users would import that module in any module they want.

Doing it this way has a second added benefit: if you abstract each web component inside of an Angular component, you'll get full IDE support: contextual documentation for inputs and components, and detection of wrong input/output names or types.

And another added benefit is it allows users to load only the subset of components they want by importing the desired modules (which is a more Angular way of doing it than invoking a global function).

By the way, there is no need to put the <script> tags in the HTML page, that can be lazy-loaded too with: import("./reveal-deps") from the Angular module.

brianlagunas commented 4 weeks ago

Just to clarify, you would recommend creating a new package called reveal-sdk-wrappers-angular.

In this package would simply be a module that handles the imports and registrations automatically. So, when a developer imports the module into their application, all the heavy lifting is done automatically.

I like this idea, and is similar to what we do for React.

I do need clarification on one thing you mentioned. You said "if you abstract each web component inside of an Angular component". Do you mean create a native angular component to wrap the existing web components and then duplicate all the properties/methods and bind them to the existing web components? Or is there a different approach you know of?

dwilches commented 4 weeks ago

you would recommend creating a new package called reveal-sdk-wrappers-angular.

Yes, I think creating a separate package for the Angular library would make the setup straightforward. This library would depend on reveal-sdk-wrappers but for the end-users it would be a transitive dependency they don't even know about.

Do you mean create a native angular component to wrap the existing web components and then duplicate all the properties/methods and bind them to the existing web components

Yes, exactly that. I don't know of a different approach than duplicating the inputs/outputs. Even when migrating from Angular.js to Angular with the code the Angular team created, us end-users had to duplicate them. From here:

image

brianlagunas commented 4 weeks ago

I'm concerned about the maintainability of duplicating the properties and methods. Especially since the RevealView has some nuisances about how certain features are enabled by setting callback functions to properties. It would add quite a bit to my maintenance overhead. Since web components do work natively in Angular, maybe if we improve the guidance around using the web components, it would be a nice balance between maintaining the library and making it easy to use.

Would shipping the RevealWebComponentsModule with just the registrations and CUSTOM_ELEMENTS_SCHEMA be a nice balance? At least this way there is no manual registration needed and it protects your code from the Angular's compiler to not warn us about legitimate errors.

Thougths?

dwilches commented 4 weeks ago

duplicating the properties and methods.

Actually only the declarations of inputs and outputs need to be duplicated, not the methods. But yes, each time a new input/output is added on the web-component, you'd need to declare it on the Angular side too.

Would shipping the RevealWebComponentsModule with just the registrations and CUSTOM_ELEMENTS_SCHEMA be a nice balance?

For end-users to use <rv-reveal-view> without CUSTOM_ELEMENTS_SCHEMA in their own modules, they'll need a declaration of what a rv-reveal-view component is, so RevealWebComponentsModule needs to export something with that selector anyways. Unless I'm missing something about the proposed solution.

brianlagunas commented 4 weeks ago

Actually only the declarations of inputs and outputs need to be duplicated, not the methods.

If I had a method/function on the web component called exportToPdf() wouldn't I need to get a ref to the web component, and then expose that method/function on the angular wrapper as well?

brianlagunas commented 4 weeks ago

Also, one more question...

If we did a native angular wrapper which copies the inputs/outputs, what do you think the name of the selector should be? Since we cannot reuse rv-reveal-view, we would need another name.

brianlagunas commented 4 weeks ago

I created a branch where I can play with ideas and possibly support your request. Feel free to review and give your thoughts. I am not an angular dev, but I know enough to be dangerous 😃

https://github.com/RevealBi/reveal-sdk-wrappers/blob/angular/packages/wrappers-angular/src/components/reveal-view/reveal-view.component.ts

dwilches commented 3 weeks ago

If I had a method/function on the web component called exportToPdf() wouldn't I need to get a ref to the web component, and then expose that method/function on the angular wrapper as well?

Yes, that's right. One option could be, instead of exposing every method that allows controlling the visualization, to expose a single "getController()" method that returns a "controller" class that has all the methods like exportToPdf, exportToPowerPoint, refreshData, enterEditMode, etc. This way there is no need to touch the Angular wrappers each time a new method is created or even updated.

Some others like editModeExited, editorClosed, dataLoading could be implemented as Outputs instead of Inputs+callbacks, and in those cases the output duplication would be needed.

what do you think the name of the selector should be

Some options that could work:

dwilches commented 3 weeks ago

Thinkering a bit about what this wrapper could look like:

Reveal-side code

reveal.module.ts

export abstract class RevealInitializer {
    abstract configure(): Promise<void> | void;
}
export const REVEAL_INITIALIZER = new InjectionToken<RevealInitializer>("RevealInitializer");

@NgModule({
    imports: [CommonModule],
    exports: [RvaRevealViewComponent],
    declarations: [RvaRevealViewComponent],
    providers: [RevealService],
    schemas: [CUSTOM_ELEMENTS_SCHEMA],
})
export class RevealModule {
}

reveal.service.ts

@Injectable()
export class RevealService {

    constructor(@Optional() @Inject(REVEAL_INITIALIZER) private revealInitializer: RevealInitializer) {
    }

    public afterInitialization(): Promise<void> {
        return import("./reveal-deps")
            .then(result => result.revealLoadedPromise)
            .then(() => this.revealInitializer && this.revealInitializer.configure());
    }
}

rva-reveal-view.component.ts

import { Component, Input } from "@angular/core";

@Component({
    selector: "rva-reveal-view",
    template: `
        <rv-reveal-view [dashboard]="dashboard"/>
    `,
})
export class RvaRevealViewComponent {
    @Input() dashboard: any;
}

This way, a customer can use Reveal like this:

Customer-side code

my-lazy-loaded.module.ts

@NgModule({
    imports: [
        RevealModule,
        ...
    ],
    declarations: [
        MyBusinessThingComponent,
    ],
    providers: [
        { provide: REVEAL_INITIALIZER, useClass: MyRevealInitializer },
    ],
})
export class MyLazyLoadedModule {
}

my-reveal-initializer.ts

class MyRevealInitializer extends RevealInitializer {
    configure() {
        RevealApi.RevealSdkSettings.setBaseUrl(SOMETHING_HERE);
        RevealApi.RevealSdkSettings.requestWithCredentialsFlag = true;
        RevealApi.RevealSdkSettings.setAdditionalHeadersProvider(SOMETHING_HERE);
        defineRevealSdkWrappers();
    }
}

my-business-thing.component.ts

@Component({
    selector: "my-business-thing",
    template: `
        <rva-reveal-view *ngIf="rvdashboard" [dashboard]="rvdashboard"/>
    `,
})
export class MyBusinessComponent implements OnInit {
    rvdashboard: any;

    constructor(private revealInitializerService: RevealInitializerService,
                ...) {
    }

    ngOnInit() {
        this.revealInitializerService
            .afterInitialization()
            .then(() => DO_BUSINESS_STUFF)
            .then(dashboardData => {
                $.ig.RVDashboard.loadDashboardFromContainer(dashboardData,
                    revealDashboard => this.rvdashboard = revealDashboard,
                    error => ...);
            });
    }
}

Further explanation of this implementation:

Finally, a possible implementation of reveal-deps is as follows:

Reveal-side code

reveal-deps.ts

import dayjs from "dayjs/dayjs.min";

(<any>window).dayjs = dayjs;

// Reveal is loaded asynchronously by adding a "script" tag to the body. This code wraps the `<script onload=...>`
// callback into a promise so the rest of our code can know when Reveal has finished loading.
const revealLoadedPromise = new Promise<void>((resolve, reject) => {
    const script: HTMLScriptElement = document.createElement("script");
    script.src = "assets/reveal/infragistics.reveal-1.7.0.js";
    script.type = "text/javascript";
    script.onload = () => resolve();
    script.onerror = error => reject(error);
    document.body.appendChild(script);
});

export { revealLoadedPromise };
dwilches commented 3 weeks ago

Something more: for the case of callbacks that are currently inputs and don't return anything, they can be converted to outputs like this (in the Reveal-Angular wrapper):

@Component({
    selector: "rva-reveal-view",
    template: `
        <rv-reveal-view [dashboard]="dashboard"
                        [dataLoading]="dataLoadingFn"/>
    `,
})
export class RvaRevealViewComponent {
    @Input() dashboard: any;
    @Output() dataLoading = new EventEmitter<DataLoadingArgs>();

    protected dataLoadingFn = (args: DataLoadingArgs) => this.dataLoading.emit(args);
}

This way customers can use it as an output instead of an input (and can use methods instead of functions without dealing with bind).

brianlagunas commented 3 weeks ago

Yes, that's right. One option could be, instead of exposing every method that allows controlling the visualization, to expose a single "getController()" method that returns a "controller" class that has all the methods like exportToPdf, exportToPowerPoint, refreshData, enterEditMode, etc. This way there is no need to touch the Angular wrappers each time a new method is created or even updated.

I have decided to just duplicate them. At first, I liked the getController function, but after playing with it I discovered that you lose the intellisense. Intellisense is critical to the developer experience. Helping you not only find the functions you'd like to call, but also provide the code commends/examples in the IDE.

Some options that could work:

I like the rva. Its simple and short. Going with that!

Thinkering a bit about what this wrapper could look like:

Thanks for the detailed examples. The approach I'm talking for these Angular wrappers is not going to be module based, but rather stand-alone components. I also do not want to make this too opinionated or veer too far from the existing jquery based RevealView and web components wrappers. I will leave the loading of the dependencies and the Angular wrapper components to the developer.

Something more: for the case of callbacks that are currently inputs and don't return anything, they can be converted to outputs like this

The problem with using outputs is that it treats them like they are events, when they are not events. This becomes an issue for a number of the callback properties because if they have a binding (which will happen with new EventEmmitter), then the RevealView behaves differently. This would then require a good amount of logic to only set the web component call back properties if the Output is observed. This will also create a divergence from the documentation. The docs will be based on the web component and not framework specific docs. So, the WC will show everything as a property, but then suddenly for Angular you have to know what's an input and what's an output. Also, the API gets a little confusing since some must remain a callback property because they return something. I think I'd like to keep things consistent across the web component wrappers and any native wrappers.

The root of this complexity comes from the jQuery based RevealView which does not use events when it should have.

I've updated the angular wrapper wiht the remaining properties and functions: https://github.com/RevealBi/reveal-sdk-wrappers/blob/angular/packages/wrappers-angular/src/components/reveal-view/reveal-view.component.ts

If this looks good to you, I can publish the initial package and see how it feels for you.

dwilches commented 3 weeks ago

but after playing with it I discovered that you lose the intellisense.

Was the getController method returning a typed object? In the case of TypeScript, as long as the function is annotated with the output type, IntelliSense will keep working and will suggest the methods of the type that is returned. In the case of plain JavaScript, not sure what it uses for type hints with IntelliSense.

brianlagunas commented 3 weeks ago

Was the getController method returning a typed object? In the case of TypeScript, as long as the function is annotated with the output type, IntelliSense will keep working and will suggest the methods of the type that is returned. In the case of plain JavaScript, not sure what it uses for type hints with IntelliSense.

No, it returned any. If I used a typed object, that would require me to keep the object types updated with new functions that are added to the underlying WC. That of course defeats the purpose of using the getController function to begin with. So, if I need to update an object type with new functions to get intellisense to work, might as well be the main Angular component 😄

dwilches commented 3 weeks ago

The rva wrapper looks good to me, I can give it a try once it's in npm

brianlagunas commented 2 weeks ago

I published the initial version to npm under the package name reveal-sdk-wrappers-angular

It uses standalone components so no module import is needed. Just import the component like RevealViewComponent or the VisualizationViewerComponent. I'm not sure about the component names 100%, I may change them to to prefix them with Rv like RvRevealViewComponent. I haven't decided yet, but this should get you started playing wiht them.

Let me know what you think.