tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

static attributes are not inherited from derived class #11

Closed sionzee closed 5 years ago

sionzee commented 5 years ago

Hello again!

I have a class what inherits another class. Where the another class has static attributes but line 163 doesn't take in account inner prototypes.

Case:

class TEST0 {
  static scope = 10;
}
class TEST1 extends TEST0 {}
class TEST2 extends Mixin(TEST1) {}
window.TEST2 = TEST2

scope is undefined at TEST2.

Here is an example of constructor to describe the issue: Class2 constructor contains:

{
  "tagName",
  "prototype": Class1
}

Class1 Constructor:

{
  "scope": 10
}

But prototype is restricted from getOwnPropertyNames and scope will be skipped.

sionzee commented 5 years ago

The same thing is also happening to functions. But it totally makes sense to me.

tannerntannern commented 5 years ago

Hi @sionzeecz, I apologize for the delay. I will take a look at this after work. I'm pretty sure I know what's going on, so it will hopefully be a quick fix. 👍

tannerntannern commented 5 years ago

Turns out it's not as simple as I had hoped... This issue isn't caused by omitting the prototype at 169 as you had suggested, the "inheritance" happens earlier. The problem is rooted in the fact that ES6 differs from other languages in how static properties are handled. Try this in the console for example:

class A { static prop = 0 }
class B extends A {}

A.prop ++;
console.log(A.prop);  // 1
console.log(B.prop);  // 1

B.prop ++;
console.log(A.prop);  // 1
console.log(B.prop);  // 2

A.prop += 10;
console.log(A.prop);  // 11
console.log(B.prop);  // 2

What's going on here? Is B.prop linked to A.prop? Sure seems like it after incrementing A.prop. But after you update B.prop, they diverge. Even if you go back and update A once again, they remain separate.

In other languages like Java, these would remain linked no matter which "entry point" got modified. This is also how static properties are linked when ts-mixer works with them. Unfortunately, it has no control over how static properties are inherited when extends is used however.

The only way I can think of to get around it at the moment would be to do something like this:

class TEST0 {
    static scope = 10;
}
const TEST1 = Mixin(TEST0, class {});
const TEST2 = Mixin(TEST1, class {});

I did release a patch though, which will be required to make this work correctly.

sionzee commented 5 years ago

Hey!

You are definitely right. This is really weird behaviour from ESM. Unfortunely I couldn't make inherited classes as mixin because they are in other modules.

I'm using inner attributes in read-only mode. So we will not modify the values.

As we are in, primitive types don't support references and are passed by values. So we should be able to read the functions or constants what are inherited.

Wouldn't be right to copy primitive values, skip the linking to original variable and keep linking to objects\arrays?

// Edit: Or we should just keep it as is it and it should do same behaviour like you wrote in your example.

// Edit 2: At Typescript page about Mixins, they're using following function:

function applyMixins(derivedCtor: any, baseCtors: any[]) {
    baseCtors.forEach(baseCtor => {
        Object.getOwnPropertyNames(baseCtor.prototype).forEach(name => {
            Object.defineProperty(derivedCtor.prototype, name, Object.getOwnPropertyDescriptor(baseCtor.prototype, name));
        });
    });
}

In my opinion, you should do the same thing in recursive way?

tannerntannern commented 5 years ago

Can you explain a little more why the solution I suggested won't work for you? I'm not sure how being in other modules would prevent it from working.

As for static property handling, I'm really hesitant to pass by value. As I said, other languages don't do it that way and technically neither does ES6. It passes by reference until the inherited value is modified. Only then does it become pass by value. This seems like an ill-defined behavior and I would prefer not to mimic it.

sionzee commented 5 years ago

Lets say you are using some kind of framework what is giving you abstract classes and interfaces. You will implement these interfaces and send it back to the framework. But the framework will throw an error because he doesn't found a static functions\constants.

For example: Framework's code (part of node_modules)

class Element {
    static create() {...}
    format() {}
}
class ShadowElement extends Element implements Formattable {}
class Registry {
   static register(element) {
      // Boom. Error if we use ts-mixer. `create` not found.
      const created = element.create();
   }
}

Now in your code you will have something like:

import {ShadowElement, Registry} from "the-framework"
class MyCustomShadowElement extends ShadowElement {
    format() {return test;}
}
Registry.register(MyCustomShadowElement);

Now. I can modify only MyCustomShadowElement, add mixin to it. But can't add it to the existing framework.

tannerntannern commented 5 years ago

After some careful thought, I've decided to not go against the grain with ES6 and just pass by value. This is technically a breaking change so I've released it as 4.0.0. This should resolve the test case that you provided at the beginning of this issue. Closing for now, feel free to reopen if you see issues with that solution.