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

feat(@Mixin): new decorator #2921

Closed
wants to merge 47 commits into from

Conversation

johnjenkins
Copy link
Contributor

@johnjenkins johnjenkins commented May 29, 2021

Much of Stencil's secret sauce comes from 'static analysis'; using the ts compiler to pull out meta from components.
With this meta, stencil can do lots of cool stuff. Make an ultra light, lazy loading layer. Make framework specific wrappers, and allow devs to make their own wrappers!

But a reliance on static analysis does come at a cost. How could the compiler derive static meta data from components that could endlessly extend for example? For this reason, extending components is not allowed in Stencil.

Because I'm a stupid sadist, I decided to see if I could find a solution: A new @Mixin decorator.
With @Mixin I leaned further onto static analysis in quite a naive way. Class members (and supporting statements) are simply merged together with the host, kinda like a cascade.

Usage

import { Component, Prop, h, ComponentInterface, Method, Mixin } from '@stencil/core';
import { Base1, Base2 } from '../bases/bases';

@Mixin(Base1)
@Mixin(Base2)
@Component(...)
export class MyInput implements ComponentInterface {
...
}
export interface MyInput extends Base1, Base2 {}

Notes:

  • You can mixin from other stencil components or more abstract classes (those that are not components)
  • @Mixin order is important! - mixin members are merged like a cascade with the host. So any name clashes (variables / methods) between Base1, Base2 and the Host will be overwritten in that order.
  • In order to get the most from typing, export an interface with the expected behaviour. In the case of name clashes use TS Omit and Pick e.g. export interface MyInput extends Omit<Base1, 'offendingMethod'>, Base2 {}
  • You can further use Omit or Pick to do just that - cherry pick incoming members.
  • In order to keep behaviour parseable and transparent, I have purposefully disallowed @Mixin nesting / recursion and trying to do so will throw an error. e.g.
@Mixin(Base1)
@Component(...)
export class MyInput implements ComponentInterface {
...
}

@Mixin(MyInput)
@Component(...)
export class MyNewInput implements ComponentInterface {
...
}

^ Error. This could be open for debate.

  • No @Component meta is mixed in. Inluding scope, shadow, styleUrl/s and assetDirs.
  • Import definitions will be brought into the host. Therefore be mindful when defining multiple mixins in a single file to not include imports that are not relevant to all mixin classes.

When writing tests I focused on 3 main use cases:

  1. Mixing in from plain, generic classes.
  2. Mixing in from other local stencil components.
  3. Mixing in from 3rd party node_modules (it'd be nice if one day, there are defacto, community generic classes to use for example)

And that's about it. I've written quite a lot of tests for this new behaviour and all seems to be working well, but there are bound to be things I've missed.

I would urge anyone who is remotely interested in this, please try it out and give feedback. This is quite a big change and it will need more eyes on it to have a chance at being accepted.

If this gets some traction, I will loop back over this when source maps are merged < I think they would be helpful*

*I have now made the necessary changes: mixins will work as expected with source maps

Fixes: #2844
Fixes: #2559
Fixes: #1127
Fixes: #1060
Fixes: #172

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • They've been added ^ here
  • Build (npm run build) was run locally

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

There is no easy way to extend from other classes.

What is the new behavior?

As described above.

Does this introduce a breaking change?

  • Yes
  • No

Manual Testing

  • Ran npm build and npm link from the root of the repo directory (on this branch)
  • Spun up a boilerplate Stencil app using the Stencil CLI npm init stencil
  • Linked my project with the instructions found in the Contributing guidelines
  • Added multiple @Mixin decorated components, with multiple diffrent type of mixin classes.
npm run start
  • All functions and builds as expected. e.g. Changing / saving Mixed in classes cause @Mixin components to reload 👍
 npm run build
  • All functions and builds as expected. Built components have all relevant mixed in class members present 👍

John Jenkins and others added 7 commits April 9, 2021 00:10
…mods.

Test transpiler - can cope with multiple files.
'omit' and 'pick' can now be used to filter real class members from the host component.
Allows for custom elements build to modify how CustomEvents, addEventListener and removeEventListeners are called.
@johnjenkins johnjenkins changed the title Feat mixin decorator feat(@mixin): new decorator May 29, 2021
@johnjenkins johnjenkins changed the title feat(@mixin): new decorator feat(@Mixin): new decorator May 29, 2021
John Jenkins and others added 6 commits June 3, 2021 01:31
…mods.

Test transpiler - can cope with multiple files.
'omit' and 'pick' can now be used to filter real class members from the host component.
… into feat-mixin-decorator

# Conflicts:
#	CHANGELOG.md
#	package-lock.json
#	package.json
@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 3, 2021

To anyone interested, I built out a temporary package of all my recent PRs (and may add some other user's PRs in future too) - just until Stencil becomes more active.

This includes:
new @Prop getters / setters (#2899)
new @Mixin decorator (#2921)
new sourceMap option for all output (including integration with @Mixins) (#2892)

You can test it out with little disruption by just swapping out any reference to @stencil/core in your package.json
e.g.

"@stencil/core": "2.6.0"

becomes

"@stencil/core": "npm:@johnjenkins/stencil-community^2.4.0"

I will keep this package in-sync with @stencil/core master up until the relevant PRs get accepted or rejected.
Many thanks!

@theo-staizen
Copy link

Thank you @johnjenkins for your hard work, you legend! Your PRs are all highly useful features!

feat(souremaps): bring in mixin source into sourceMaps
@agustiza
Copy link

agustiza commented Jun 10, 2021

This PR is truly impressive and I really don't want to be a party pooper but here comes the but
https://reactjs.org/blog/2016/07/13/mixins-considered-harmful.html

As cool as this PR might be I don't believe Mixins are the way to go. Stencil would only be repeating a very costly mistake the react community tried and already failed at. The article goes in detail on why Mixins might cause headaches on large codebases and those cases are pretty applicable to a Stencil codebase.

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 10, 2021

hey @UnGaucho! Thanks for the thoughts and feedback, really appreciated.
I do understand the concerns and I am aware of React's history with Mixins, however, I do believe this PR is a different beast of the same name; designed to be a simple 'dev quality of life' solution, not a framwork architecture. Let's see if I can't convince you :D :

  1. Mixins introduce implicit decencies

"Sometimes a component relies on a certain method defined in the mixin, such as getClassName(). Sometimes it’s the other way around, and mixin calls a method like renderHeader() on the component. JavaScript is a dynamic language so it’s hard to enforce or document these dependencies."

A mixin cannot call anything on a component. The cascade is one way. At the end of the day the generated JS is not dynamic from mixin to component - it is simply as if the mixin is part of the component.
This leads on to the the other strength of this PR (and stencil more generally) is that everything is based on typescript. If you change anything in a the Mixin that any one component relies upon, typescript will let you know. Which leads to:

" A component’s render() method might reference some method that isn’t defined on the class. Is it safe to remove? Perhaps it’s defined in one of the mixins. But which one of them? You need to scroll up to the mixin list, open each of those files, and look for this method."

Once again, not a problem. Because we define in typescript what class is extending from which Mixin class (and even which Mixin members from where), intellisense takes care of the rest. I click on the 'method' that isn't defined in my class and it takes me to the Mixin file / line where it is defined.

"Often, mixins come to depend on other mixins, and removing one of them breaks the other. In these situations it is very tricky to tell how the data flows in and out of mixins, and what their dependency graph looks like. "

As mentioned in the readme above, I have disallowed mixin recursion / nesting for this reason.

  1. Mixins cause name clashes

Not in this PR they don't. Typescript won't let them :)

Typescript will complain that there are name-clashes, then it is up to the developer to choose (as described ^ via Omit and Pick) which members from which classes they actually want. In this PR, not only does this make the typescript error go away, but (via static analysis) that is actually what will happen.

  1. Mixins cause snowballing complexity

I cannot speculate how or what people will do with the PR, but I have tried to limit what they can do with the purposeful blocking of mixin recursion / nesting which (I believe) should help with this no end.

@agustiza
Copy link

agustiza commented Jun 10, 2021 via email

@StevenMeyer
Copy link

I really want to see real TypeScript mixin support.

johnjenkins and others added 7 commits September 6, 2021 00:34
# Conflicts:
#	.prettierignore
#	package-lock.json
#	package.json
#	scripts/license.ts
#	scripts/release-tasks.ts
#	scripts/release.ts
#	scripts/utils/release-utils.ts
#	src/compiler/transpile/transpile-module.ts
#	src/runtime/test/fixtures/cmp-b.tsx
#	src/sys/node/node-sys.ts
#	test/browser-compile/src/utils/css-template-plugin.ts
#	test/karma/stencil.config.ts
#	test/karma/test-app/slot-fallback/karma.spec.ts
#	test/karma/test-app/util.ts
@perronef5-ukg
Copy link

Hi Any update on this PR? I could really use this now! 😅

@Alifarmand
Copy link

What is the status of this?
I am waiting for so long for this PR.. :(

@perronef5-ukg
Copy link

@johnjenkins You think this PR will go in anytime soon?

@johnjenkins
Copy link
Contributor Author

@perronef5-ukg no idea bud - sorry.
It's a monster PR and the new team have got their own priorities... I can ask if it's on their radar

@a-giuliano
Copy link
Contributor

Hi folks! Thanks for your interest in this PR. We’ve got it on our internal backlog to investigate the feature’s feasibility, but we haven’t made any decisions on it yet. We'd love it if you could react to the PR summary with a '👍' to give us a signal that we should reprioritize.

John Jenkins added 4 commits November 25, 2021 23:35
# Conflicts:
#	package-lock.json
#	package.json
#	src/compiler/build/compiler-ctx.ts
#	src/compiler/transformers/transform-utils.ts
#	src/compiler/transpile/run-program.ts
#	src/compiler/transpile/transpiled-module.ts
#	src/declarations/stencil-private.ts
#	test/karma/package.json
@Perronef5
Copy link

@a-giuliano Any update on this PR?

@a-giuliano
Copy link
Contributor

Thanks for checking in on this, @Perronef5. While I don't have any major updates, I can say this feature will be on our roadmap for our first quarter of 2022 (that's February - April for us). I know the community is really excited about this feature, so we really appreciate your patience!

@jptcnde
Copy link

jptcnde commented Feb 5, 2022

excited about this change tho. any news on this guys?

@venkatesh-nagineni
Copy link

Waiting from long time for this feature, any update on release?

@FirstVertex
Copy link

let's go! it's overdue

@mariohamann
Copy link

Still April, still hoping for this one. ❤️

@a-giuliano
Copy link
Contributor

Hi everyone! Thank you all for your interest in this PR. I know in my last update I mentioned that this feature will be on our roadmap for our first quarter of 2022. Unfortunately, we haven't yet made any decisions on this PR. We recognize that a number of community members are interested in this PR, and we sincerely appreciate your patience.

@CraigDeemingUP
Copy link

After trying to extend / mixin Stencil components I've end up here. Really looks like the solution close, do you chaps have an estimate on when / if this feature will be merged?

@johnjenkins
Copy link
Contributor Author

johnjenkins commented Jun 30, 2022

After investing much more time in the whole stencil 'stack', I have come to feel that this PR is probably not the best way forward for stencil. I use my fork of stencil every day which includes the @Mixin decorator and have yet to reach for it.

  1. I find the actual use cases to be few and far between; they do crop up but (aside from shared properties,) can mostly be solved in other ways - component composition, functional components, call invocation, shared utility functions etc

  2. The solution proposed is just a little too novel and proprietary. I feel one day, stencil could, migrate from it's tight coupling with typescript's AST static analysis - the current decorators could (somehow) be migrated to actual runtime js functionality. I don't feel the same relates to @Mixin

  3. The solution proposed does not deal satisfactorily with 3rd party stencil libs. I said at the time that this was a main use-case I hoped to serve, but the way it's implemented is too fragile and unwieldy. @Mixin relies on pulling out ts class members (.e.g. @Prop() something), and in-order to do that from a 3rd party lib you'd have to include the dependency as a repo (to get access to the original ts). Trying to mixin from, for example @3rdPartyStencilLib/dist/collection/thing does not work and would require a substantial re-write.

I personally would like to see reactive controllers to be implemented within stencil. Once again, would not solve shared properties but would solve many other pain points.

I thank all those who gave kind words of encouragement and apologise for any disappointment caused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment