Skip to content
This repository has been archived by the owner on Oct 1, 2021. It is now read-only.

Setter-only design vs. constructor arguments #8

Closed
domenic opened this issue Apr 5, 2018 · 5 comments
Closed

Setter-only design vs. constructor arguments #8

domenic opened this issue Apr 5, 2018 · 5 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Apr 5, 2018

https://github.com/valdrinkoshi/virtual-list/blob/design/DESIGN.md has a lot of examples where you construct various classes by creating empty versions of them and then setting a bunch of properties with Object.assign(). This is strange in a few ways:

  • It implies that the objects are fully functional even before these properties are set. Is that true? I find it hard to believe that a VirtualList with no layout or items is still a fully-functional VirtualList.
  • It implies that these properties can be changed at any time and the class will respond appropriately. (E.g., changing container.)
  • It implies that these properties can all be changed independently of each other. (Maybe this is true, I'm not sure.)

Instead I would suggest making these constructor arguments: new VirtualList({ layout, container, items, newChildFn }), etc. This ensures that you cannot construct an instance of the class without setting them, and setting them all at once, together.

This also allows more fine-grained separation so that things which can be set and modified later and independently (say, VirtualRepeater's first?) are exposed as public getter/setter pairs.

@valdrinkoshi
Copy link
Collaborator

Constructor arguments serve well instantiations like this:

const Repeats = Superclass => class extends Superclass {
  constructor(config) {
    super();
    this._container = config.container;
    this._container.style.position = 'relative';
  }
};

class MyList extends Repeats(class {});

const list = new MyList({
  container: document.body,
  // ...
});

But this won't work for custom elements because it forces them to have container defined at construct time, e.g. if we want to set container to be this or this.shadowRoot:

class MyList extends Repeats(HTMLElement) {
  constructor() {
    super({
      container: this, 
    });
  }
}

customElements.define('my-list', MyList);

@valdrinkoshi
Copy link
Collaborator

Uhm, I guess the custom element could just define getters for these properties and that's it, e.g.

class MyList extends Repeats(HTMLElement) {
  get _container() { return this; }
}

@domenic
Copy link
Collaborator Author

domenic commented Apr 6, 2018

As far as I know none of the classes currently in DESIGN.md are custom element classes, so I'm not sure why the concern applies...

@valdrinkoshi
Copy link
Collaborator

valdrinkoshi commented Apr 6, 2018

Yep, but I was working on updating to constructor arguments and realized that this doesn't work well for custom elements, e.g. the <html-spec-viewer>
https://github.com/valdrinkoshi/virtual-list/blob/707eef6d8264e1b373b42b868d18b63d7aa30cc3/demo/html-spec/html-spec-viewer.js#L6-L30

This would have to be changed to something like this to work with constructor arguments:

class HTMLSpecViewer extends HTMLElement {
  constructor() {
    super();

    this.style.display = 'block';
    this.style.minHeight = '100000px';

    const htmlSpec = new HtmlSpec();
    htmlSpec.head.style.display = 'none';
    this.appendChild(htmlSpec.head);
    this._htmlSpec = htmlSpec;

    this._list = new VirtualList({
      items: [],
      container: this,
      layout: new Layout({
        itemSize: {
          y: 10000,
        },
        _overhang: 800,
      }),
      newChild: (item) => item,
      recycleChild: () => {},
    });
  }

@domenic
Copy link
Collaborator Author

domenic commented Apr 6, 2018

Yeah, that makes a lot of sense to me. Prefer composition over inheritance, after all.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants