Skip to content

Allow mixins to describe custom elements #31

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

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Allow mixins to describe custom elements #31

merged 8 commits into from
Feb 12, 2021

Conversation

justinfagnani
Copy link
Collaborator

@justinfagnani justinfagnani commented Nov 13, 2020

Addresses feedback in runem/web-component-analyzer#157

* ```
*
* Is described by this JSON:
* ```json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it still have a "superclass" field that has as value base here? Thats how I currently handle it in @custom-elements-manifest/analyzer, but it might be redundant I guess?

Copy link
Collaborator Author

@justinfagnani justinfagnani Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - mixins (usually) don't have superclasses until they are applied to a specific superclass.

When mixins do have a superclass, it's because of mixin composition:

const MixinA = (base) => class extends base {
  fieldA;
}

const MixinB = (base) class extends MixinA(base) {
  fieldB;
}

class C extends MixinB(S) {
}
const c = new C();
c.fieldA; // exists
c.fieldB; // exists

So here the superclass of MixinB would be MixinA.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a short mention of this in the docs

Copy link
Collaborator

@thepassle thepassle Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a mixin has nested mixins, though, are those nested mixins then documented as being mixins, or superclasses? E.g.:

const MixinA = (base) => class extends base {
  fieldA;
}

const MixinB = (base) => class extends MixinA(base) {
  fieldB;
}

Would the mixin declaration look like this:

        {
          "kind": "mixin",
          "description": "",
          "name": "MixinB",
          "mixins": [
            {
              "name": "MixinA"
            }
          ],
         "parameters": [
             {
               "name": "base"
             }
           ],
          "members": ["//etc"]
        }

If we consider MixinA to be the superclass in that case, we have to change superclass to be superclasses and be an array, because there can be multiple mixins applied.

Intuitively, I feel like I'd prefer to name "nested mixins" just mixins in this case though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, I'll change the comments to says composed mixins should go in the mixins field.

schema.ts Outdated
@@ -427,6 +427,9 @@ export interface ClassMethod extends FunctionLike {
* argument, so consumers of this interface should assume that the return type
* is the single argument subclassed by this declaration.
*
* A mixin should only have a superclass if it composes another mixin, and the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure I understand.

Given:

export const MixinA = base => class extends MixinB(base){}

Here the MixinB would be "superclass"?

What in the case of:

export const MixinA = base => class extends MixinC(MixinB(base)){}

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was updated to say "a mixins should not have a superclass". In your examples the other mixins would be included in the mixins field of MixinA.

@thepassle
Copy link
Collaborator

I would still like to clarify this before merging #31 (comment)

@thepassle
Copy link
Collaborator

Hey @justinfagnani now that all the holidays are behind us, any chance we could pick up this discussion again? 🙂

@thepassle
Copy link
Collaborator

Nice, LGTM 👍

@justinfagnani justinfagnani merged commit 14aad79 into master Feb 12, 2021
@justinfagnani justinfagnani deleted the mixins branch February 12, 2021 18:30
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.

3 participants