Skip to content
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

Error On Any Inheritance #1060

Closed
lupiter opened this issue Sep 4, 2018 · 38 comments
Closed

Error On Any Inheritance #1060

lupiter opened this issue Sep 4, 2018 · 38 comments
Assignees
Projects

Comments

@lupiter
Copy link

lupiter commented Sep 4, 2018

Stencil version:

 @stencil/core@0.12

I'm submitting a:

[X] bug report

The change in 0.12 to introduce a compiler error when a Stencil component inherits from any class is causing us a lot of problems. Most of our components inherit from non-Stencil base classes that define common properties (not @props) and methods.

While I can understand that supporting inheritance between components is hard, blocking all inheritance seems like a poor solution. Can the error be changed to stop you inheriting only from other classes that are also annotated with @component and leave open the option of inheriting from non-component classes? At the very least can we get some kind of flag or option to disable this check? As it stands we can't move to 0.12.

@ionitron-bot ionitron-bot bot added the triage label Sep 4, 2018
@jthoms1
Copy link
Member

jthoms1 commented Sep 6, 2018

Currently we do not allow for any inheritance of Stencil Components. We have talked about possibly allowing for mixins in the future. But this will need to be outlined in our future roadmap.

I am tagging this as a feature request. Thanks!

@jthoms1 jthoms1 added the feature label Sep 6, 2018
@ionitron-bot ionitron-bot bot removed the triage label Sep 6, 2018
@kael
Copy link

kael commented Sep 6, 2018

The lack of inheritance is a bit problematic from a productivity point of view.

Copying and pasting the same set of code, for example for click events, to different components, and then readjusting when necessary is time consuming, and then you have to keep track of changes and modify all components after one modification.

But well, there seems to be strong reasons against it.

Perhaps there could be more patterns examples for composition (until with get some cool stuff) ?

@lupiter
Copy link
Author

lupiter commented Sep 6, 2018

I'm not sure this is a feature request? Maybe I didn't explain what I meant properly. I understand that the following doesn't work:

@Component(...)
class Animal {}

@Component(...)
class Dog extends Animal {}

but we have

class Animal {}

@Component(...)
class Dog extends Animal {}

Our base class then handles our generic service interactions (in our case with the redux store).

This did work prior to 0.12, when the compiler error was introduced.

@jthoms1 jthoms1 self-assigned this Sep 10, 2018
@jthoms1 jthoms1 added this to Backlog 🤖 in Stencil via automation Sep 10, 2018
@jthoms1
Copy link
Member

jthoms1 commented Sep 10, 2018

@lupiter I think your use case is valid and we should allow for it.

@jthoms1 jthoms1 added bug and removed feature labels Sep 10, 2018
@jthoms1 jthoms1 moved this from Backlog 🤖 to On deck ⚾️ in Stencil Sep 10, 2018
@jthoms1
Copy link
Member

jthoms1 commented Sep 13, 2018

@lupiter could you provide the base class that you wanted to extend. We are trying to identify solutions to the problem. I just want to have better insight. Thank you!

@lupiter
Copy link
Author

lupiter commented Sep 13, 2018

@jthoms1

Here you go. It should remind you of stencil-redux but because we don't have a root component (we're making a component collection) we setup the redux store with a special <sw-store> component. getSubscription is related to configuring a redux middleware.

import { ActionType, AppState, Store, StoreStates, Subscription } from 'global/state';
import { extractFromGlobal } from 'global/store';

export abstract class SWComponent {
  public abstract store: Store;
  public abstract storeManager: any;
  public abstract applyState(state: AppState): object;
  public abstract storeState: StoreStates;
  protected subscriptionId: string;

  public linkStore(): void {
    if (!this.store) {
      this.store = extractFromGlobal();
    }
    this.store.mapStateToProps(this, (state: AppState) => this.applyState(state));
    if (!this.subscriptionId) {
      this.subscriptionId = this.store.getId();
    }
    this.store.dispatch({ type: ActionType.UPSERT, payload: [this.getSubscription()] });
    this.store.mapDispatchToProps(this, this.getSavingProperties());
  }

  protected getSavingProperties(): object {
    return {};
  }

  protected getSubscription(): Subscription {
    return new Subscription(this.subscriptionId, []);
  }

  public willLoad(): void {
    if (this.storeManager && this.storeManager.componentOnReady) {
      this.storeManager
        .componentOnReady()
        .then(() => {
          this.linkStore();
        })
        .catch((reason: string) => {
          console.error(reason);
          if (window.name !== 'nodejs') {
            throw new Error('Couldn\'t find a <sw-store> element, May be misconfigured.');
          }
          this.linkStore();
        });
    } else if (window.name !== 'nodejs') {
      throw new Error('Couldn\'t find a <sw-store> element, May be misconfigured.');
    }
  }
}

@dgibson666
Copy link

dgibson666 commented Oct 2, 2018

I've just updated to .13* and it's broken what was previous working in a basic extends case. Not doing anything fancy - just trying to provide a handful of utility methods in a parent class so I'm not repeating code and creating a maintenance nightmare. Please allow this once again. Thanks.

@safebyte
Copy link

safebyte commented Oct 3, 2018

@lupiter I think your use case is valid and we should allow for it.

I do not understand why inheriting from an imported class is considered valid, but the inheritance of a component isn't. IMHO this should be exactly the other way around. Handling dependencies on a component basis seems to be much more convenient than including / shipping global dependencies from external files.

Although there had been plenty of feature requests for component inheritance, there hasn't been a clear statement. The most promissing issue has been closed due to inactivity while waiting for an answer from @jthoms1 , others just refer to the closed one.

The ability to inherit from a component is a key feature to me and it seems like others base their decision of working with stenciljs on this aswell. Will stenciljs ever support inheritance of components? If so, does a roadmap exist?

My use case is a "window" component that comes in different variations: <win-alert/>, <win-confirm/>, <win-prompt/>... they all have to inherit the base functionality from <win-base/> which comes with the window container, a title bar, close / minimize / maximize buttons and some global APIs for handling events, a taskbar etc.

Thanks.

@peterpeterparker
Copy link
Contributor

@jthoms1 are you still looking for practical examples where inheritance would be a good solution/welcomed?

if so, I could provide one soonish, I'm duplicating code in 3-4 components of the same project right now and I miss having the ability of having an abstract class ;)

@val-o
Copy link

val-o commented Nov 29, 2018

Any update on that one? We have to duplicate code because of this error...

@EricSeastrand
Copy link

We need this as well: we want to use the <ion-back-button /> component, but slightly adjust its behavior when clicked.
The only way I see to go about this would be to copy/paste large portions of the ionic component into my own. Unfortunately, that component depends on things like the createColorClasses() function, which can only come from @ionic/utils/theme.

Our specific use-case actually arose because we're trying to include a stencil PWA within a larger Ionic app that targets native devices. The <ion-anchor> and other ionic links don't know which router to interact with, which causes issues, so we need to handle that click behavior/routing within the PWA separately to avoid conflicts.

@fsodano
Copy link

fsodano commented Jan 4, 2019

Hello there, while we wait for a solution for this...

This may not be the perfect solution, since MDN recommends against using Object.setPrototypeOf(), but at least it provides an alternative.

Since classes are just syntactical sugar (i.e. JS it still remains prototype based), we can leverage this and change the prototype of a stencil component, to be the prototype of our base component.

import {Component, Element} from "@stencil/core";
import {BaseComponent} from "../../base-component";

@Component({
    tag: 'ti-form',
    styleUrl: 'form.css'
})
export class Form {
    @Element() el: HTMLFormElement;

    render() {
        return (
            <form>
                <slot/>
            </form>
        )
    }
}

Object.setPrototypeOf(Form.prototype, BaseComponent.prototype);

This has its limitations (for instance, you can't use super).

Another alternative, if you are just wanting to reuse some specific methods, you may opt for doing something like:

Form.prototype.componentDidLoad = BaseComponent.prototype.componentDidLoad;

or simply

@Component({...})
export class Form {
    // ...
    componentDidLoad = BaseComponent.prototype.componentDidLoad;
    // ...
}

@daniell0gda
Copy link

daniell0gda commented Jan 13, 2019

@fsodano Thank you for this input. As helpfull it is, it's still pretty ugly.
Do you have guys (@jthoms1) any timeline for this feature/bug?

@pyummm
Copy link

pyummm commented Feb 15, 2019

@fsodano Thanks, works, but as said here above, pretty ..
And again, if any timelines for inheritance?

Do builder with lots drag-drop elements, and have manually add same drag-drop methods to many components, looked badly, so use this crutches, and started to await

@felschr
Copy link

felschr commented Mar 4, 2019

Inheriting from a base class without any component decorators should really be allowed imo.

@JakobJingleheimer
Copy link

Bump. It's been 6 months since @jthoms1 agreed above that this is valid and switched it from feature to bug.

@jsmith-sapient
Copy link

I looked into creating a PR for this, but I've found some seemingly bizarre (and seemingly unnecessary) behaviour that's getting in the way: If an exported class is not decorated with @Component

  1. it's ignored
  2. when the ignore is disabled, its other decorators are stripped off

I disabled both of the above, and everything seems to work just fine. But the behaviour is so unexpected, it makes me think there's a reason (but there's no comments about it in code, nor in the PR that introduced them). I'm not too familiar with the source of Stencil, so perhaps it's obvious to someone more familiar?

Also, why is double-decorating not allowed? I would think it perfectly fine, and subsequent decorations would just stomp previous ones where they collide. I tried doing that (after disabling those checks), and it seems to work fine.

@jsmith-sapient
Copy link

@adamdbradley could you provide any context?

@nielsboogaard
Copy link

@manucorporat does that label means it's fixed already, and if so in what release? I couldn't find any PR related to this.

@RienNeVaPlus
Copy link

RienNeVaPlus commented Apr 4, 2019

@nielsboogaard have a look at the branch core-refractor, also these breaking changes might be relevant. For more details visit stencil's slack.

@FRSgit
Copy link

FRSgit commented Apr 24, 2019

Is there any release date for stencil one?

Do you know maybe how to get to the stencil's slack?
image

@peterpeterparker
Copy link
Contributor

@FRSgit regarding Slack: https://stencil-worldwide.herokuapp.com

@manucorporat
Copy link
Contributor

Stencil 1.0 will not allow any inheritance of component classes, cuz stencil might need to apply transformation of the base class. In addition, allowing inherintance opens the world to a completely new world of problems, since the compiler will most likely not be able to static analyze the prototype chain at compiler time.

Stencil might emit components that works in workers, on extend directly from HTMLElement, or even angular components directly, it needs to be able to change the base class.

We have got a lot of feedback about how to build components and reuse code, we have found that there are good pattern that can replace the need for inheritance. We will extend docs with more information about code sharing between components soon!

Stencil automation moved this from On deck ⚾️ to Done 🎉 Apr 25, 2019
@FRSgit
Copy link

FRSgit commented Apr 25, 2019

Okay, the only @peterpeterparker ah, sorry, didn't saw that on readme page, my bad.
@manucorporat That's a pity, but understandable. Are there any plans on supporting extending native elements though? Im' talking about components defined like here.

Supporting this approach would give a possibility to write better (semantic-wise) html code, so as a result also a real boost for all SEO-connected usecases.

@astral-arsonist
Copy link

@manucorporat Did those docs about code sharing between components get written? Would love a link if so!

I'm specifically wondering how I can take an existing Ionic element and modify it to have some custom behaviors, without having to copy/paste and essentially fork the entire element. If this is not possible, I likely will not be able to adopt Stencil for my project.

@menosprezzi
Copy link

menosprezzi commented Aug 17, 2019

Hi Guys!
I'm was working on a large scale internal Design System and developed a pattern for creating components that extend functionality. It is based on Polymer's behavior mixin pattern, but it's cleaner.

Suggestions please!

https://gist.github.com/menosprezzi/9d4de98960d343a4283e268073402b6c

In this example we have a layout component that controls a navdrawer that closes if focus change.

I'll revise this pattern and write an Medium article soon as possible explaining the concept

thanks!

@ghost
Copy link

ghost commented Aug 21, 2019

Is there any best practice to achieve the same behavior as inheritance provides? It's a pretty big problem for me, I don't want to repeat the same code in all my classes...

@Danetag
Copy link

Danetag commented Oct 16, 2019

@manucorporat Hi! Do you have more information about how to share code between Stencil components? Thanks!

@supproduction
Copy link

supproduction commented Oct 29, 2019

I found workaround to share code between Stencil components. But in any case it inconvenient.
You can use use decorator from typescript-mix library and also duplicate all props and state to the new component.

import { use } from 'typescript-mix';
import { Component, Prop, State } from '@stencil/core';
import { ComponentToOverride } from '../path';

export interface OverrideComponent extends ComponentToOverride {}
 
@Component({
    tag: 'override-component',
    styleUrl: 'style.scss',
    shadow: true,
})
export class OverrideComponent {
    @use(ComponentToOverride) this: ComponentToOverride;

    @State() stateValue: string;

    @Prop() name: string;
}

And also you should avoid arrow functions in the component

@META-DREAMER
Copy link

@manucorporat
Any update yet on how to achieve code sharing between components? This is a huge non-starter for our team, and IMO an essential feature for something like Stencil aiming to be the best way to build design systems. Simply not scalable without it.

@adamdbradley You mentioned using utility functions in this issue, is there any examples or docs on how this can be accomplished? #936

@dgesteves
Copy link

I have a question regarding this subject.
I want that all my library components have a theme property or for example RTL support or something that will be present in all components.
How can i use inheritance like class CarCompoent extends VehicleComponent?
I sow @jthoms1 saying that we should use composition but how can i do that?
using a component and then a to be able to put other components inside?
But in this case i can't style the slot deeply.

Can someone help me with this issue?

@chiqui3d
Copy link

So what's the solution for reusable code if we can't use inheritance?

@robmarti
Copy link
Contributor

I'm just going to leave this here.
Basically it says :

  • If one component is some kind of special case of another more generic component (e.g. WelcomeDialog and Dialog), then let the "special" component render the "generic" component and configure it with properties
  • If you want to reuse non-UI related functionality, create a separate JS/TS module and import it within your components (I guess this is what @adamdbradley meant in Component Inheritance #936 when he talked about using utility functions)

@chiqui3d
Copy link

chiqui3d commented Jan 25, 2020

Just because Adam Abramov doesn't like the inheritance, doesn't mean we all have to follow the same pattern, if in the end we're going to follow the same React pattern, can you tell me what I want to use Stencil for? --> Nextj.js

@robmarti
Copy link
Contributor

I guess component composition is a proven pattern and a more flexible alternative to inheritance.
Just because React and Stencil follow a similar approach when it comes to reusing certain functionality doesn't mean they do the same thing. But I'm not here to argue, I just wanted to provide you with a possible solution for reusing functionality without inheritance.
Have a nice one !🙂

@META-DREAMER
Copy link

Composition alone isn't enough when you want to be able to share / pass through a common set of props without having to duplicate those props for every single component. I use composition to build my design systems with styled-system, its definitely the way to go, but its not feasible if I have to repeat the same set of 20-30 common styling props on every component (color, margin, padding, etc)

@OddDev
Copy link

OddDev commented Oct 26, 2020

Would be cool to have inheritance implemented. I have a certain use-case where the code would be so much cleaner and easier to maintain.

@FirstVertex
Copy link

FirstVertex commented Feb 14, 2023

Some have asked how to implement the composition solution that was mentioned. Here is what I have done.

Consider a set of date-picking controls. One is DatePicker and a similar one is DayOfYearPicker, and we have DateRangePicker. They have similar needs from a UI standpoint, much can be shared as far as presenting a date, opening a dropdown, navigating a calendar, representing selection, etc.

I made a contract which the Stencil Date Components must fulfill:

export interface IDateComponent<TValue> {
    value?: TValue;
    valueChange?: EventEmitter<TValue>;
}

Now I made this Stencil component for the DatePicker, it implements this IDateComponent interface. I wrote the date component entire logic here. And then I transferred it to a separate class which is not a Stencil component. I left only the Stencil decorators and lifecycle hooks in the Stencil component. Everything else I call into this new "base class". In Stencil component ctor I instantiate base class. So I called it "base class" this is just a name. It is not inherited but it treats it the same way you would "super". This is what we mean by "composition", the base is just a class composed with the component.

@Component({
    tag: 'ace-date-picker',
    shadow: true
})
export class AceDatePicker implements IDateComponent {
    @Prop({ mutable: true }) value?: IAceDate;
    private _base: DateComponent;

    constructor() {
        this._base = new DateComponent(this);
    }

    componentWillLoad() {
        this._base.componentWillLoad();
    }

    componentDidLoad() {
        this._base.componentDidLoad();
    }

    componentWillRender() {
        this._base.componentWillRender();
    }

    render() {
        return this._base.render();
    }
}

You see this is just a flyweight wrapper now, a facade around the real class, where I do not have Stencil's inheritance limitations.

DateComponent, now is just what used to be in the AceDateComponent. But from there once again I moved all of that logic to a DateComponentBase, an abstract class. This is where all of the display logic ended up. Just put it in a new .tsx file, you can import anything you want here and e.g. render jsx markup, it's not going to bother inheritance.

abstract class DateComponentBase<TValue = IAceDate> {
    constructor(public component: IDateComponent<TValue>) { }

    componentWillLoad() { }

    componentDidLoad() { }

    componentWillRender() { }

    render() { }
}

Now in my base component I can touch the Stencil component using the injected this.component. It has a contract that I can use to get the value, or emit an event, or e.g. touch a prop decorated with @State to force a rerender.

protected _setValue(date: TValue) {
    this.component.value = date;
    this.component.valueChange.emit(date);
}

Just write "this.component" instead of "this."

And I have a few classes who extend that. These are the ones the components are composed with:

export class DateComponent extends DateComponentBase<IAceDate> {
}

export class DayOfYearComponent extends DateComponentBase<IAceDayOfYear> {
}

export class DateRangeComponent extends DateComponentBase<IAceDateRange> {
}

Now I make regular Stencil component for AceDayOfYearPicker and AceDateRangePicker. They have a duplicate of the wrapper above I made for AceDatePicker. This is where we're forced to duplicate some code, but that has been minimized to only the props, and lifecycle hooks, anything that needs a Stencil decorator, needs a stub here, which will always be 1 line that calls into the composed "base" class. The only difference is which base do they new in their ctor. AceDayOfYearPicker is composed with a "base" of DayOfYearComponent, and AceDateRangePicker is composed with DateRangeComponent, and these bases inherit from the abstract DateComponentBase.

Hope this can help. ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Stencil
  
Done 🎉
Development

Successfully merging a pull request may close this issue.