Open jasonmacdonald opened 6 years ago
Thanks for taking the time to write this up 🙂
Contributions
I know making Marko easier to contribute to has been on @DylanPiercey's mind since he joined the team - and he's made some great progress on our test suite. It's a lot easier now to understand how to add tests, plus the suite runs about 5x faster than it used to. (We probably should update the CONTRIBUTING section on adding tests...)
There's still (a lot) more we can do to improve in this area and if you have any specific thoughts on this, we should definitely discuss them.
Also, if you (or anyone else) is wanting to contribute, but has questions about the codebase, we're happy to answer them and jump into a call if need be.
Events
As Patrick mentioned in https://github.com/marko-js/marko/issues/624#issuecomment-332365463, passing a function to event attributes is more feasible since the changes in 4.3
. I haven't really taken a look, but I think the changes to support it would be pretty minor.
However, I'm not sure passing functions down from parent to child is the way to go. "Data down, events up" is the recommended flow for working with Marko components.
@gilbert's been an advocate of using the emit
function to concisely bubble up events:
class {}
<for(color in input.colors)>
<div on-click('emit', 'color-selected', color)>${color}</div>
</for>
Then in the parent:
class {
onCreate() { this.state = { color:null }; }
setSelectedColor(color) { this.state.color = color; }
}
<div>
<color-list colors=['red', 'blue', 'green'] on-color-selected('setSelectedColor')/>
The current color is ${state.color}
</div>
(And back to contributions/community, we need to make it easier to share live examples like this. Try online is almost there)
Another thing I'd like to see on the events front is to make it easier to pass through events and we don't have a good way of doing this right now. This is most helpful when wrapping another component.
Say I'm creating a <fancy-button>
component. All it does is add .fancy
class and should delegate everything else to the DOM <button>
. We can spread input
into the <button>
, but this doesn't include events 🙁
<button.fancy ...input/>
Animations
Better animation support is something we keep bringing up, @DylanPiercey and I were just talking about it today. I really want to see a better API for animating nodes/components in and out, but I'd also like to see support for preserving components across different parts of the component tree and animating them to new locations.
https://github.com/dev-labs-bg/transitioner https://css-tricks.com/animating-layouts-with-the-flip-technique/
Back on events, #624 would make it easier to reuse functionality for events like attach/detach:
import { animateIn, animateOut } from '@jasonmacdonald/marko-animations';
<div on-attach(animateIn) on-detach(animateOut)>
Hello World
</div>
This might be a good place to start: experimenting in user-land.
Devtools
Yes. I think we might even be able to make Marko work with the React DevTools 😆
More thoughts to come later, but I wanted to go ahead and get something out there
Hey Michael, thanks for the response. Glad to hear I'm not the only one thinking on these things. I'll admit, I was a bit frustrated yesterday when I wrote the above (after banging my head against the wall for about 5 hours), so please take it with a grain of salt.
On the events, I don't disagree. I just constantly find use-cases where I have to write a lot of boilerplate in order to accomplish something fairly simple (in my opinion). I think option 2 in my example is actually a much better approach. By giving the parent component the ability to listen to child events when transclusion is used, you accomplish exactly what you would do if tab component lived explicitly inside the tablist component's template, rather than it being passed in as bodyRenderer. I keep tripping up on this because the <include>
should be able to be treated exactly as if I had placed <tab>
in the template instead of <include>
.
If you could at least listen to events on the <include(input.renderBody) on-click('setClicked')>
that would go a long way to being able to keep that state logic inside tablist, instead of outside of it. This is what I was trying to understand yesterday, why does the event emitted by "tab" not get captured by the <include/>
tag - I'm still trying to figure that one out, it seems that the tag just passes a multi-dimensional array [['click', 'click',false]] but never triggers the listener attached to it.
On the contribution question, I do want to contribute and I am more than willing to make the changes I have talked about, but it's very difficult right now to narrow down specific questions I could ask. Some of the things I would like to contribute seem to be spread across many, many areas of the code base so I don't really have a firm enough grasp of all the internals to not just ask "how do I do this?" :) I'm trying to get there, but time is always my enemy as I have an entire team of dev's just trying to build stuff, so I can only poke around the code now and then (I'm sure we have all been there). I think if someone could put together even a high-level diagram of the different steps (code/location) that the build and then runtime goes through, it could go a long way into helping people better understand the codebase. Even at a high-level something like
Some details around how things like codegen.addStaticVar
or builder.require
contribute to the final built file would also be helpful. I understand it at a high level, but I don't have a deep enough understanding yet how that may affect the final outcome of the built file. The specific order of operations is still a bit fuzzy. Also, details around the differences between things like builder
, codeGen
, transform
, and so on would help. Right now, most of what I have learned has been from reading the taglibs/core directory but figuring out exactly when each of those is called and how they relate to things defined in the 'marko.json' file (node-factory, code-generator, transform, renderer, autocomplete??) is also unclear - and by unclear I mean the timing and what affect it has on the outcome. I'm sure it all makes sense to the Marko folks, but from the outside it's a lot to try and wrap your head around when we don't have an in-house expert to poke for questions :)
Funny story, I actually had one of my dev's say to me the other day, "why does Marko only allow two kinds of attributes for custom components, 'string' and 'expression'? I really want to use a boolean!" I said, "what do you mean? You can use boolean, integer and more!" Then he sent me this link and I realized he had no way of knowing those things existed. I knew because I had been through the code. :) I think I'll submit an update for the doc later today lol.
Anyway, just a thought. I might try to take a stab at it, and just get you guys to review/edit it.
Apologies for the long post.
NOTE: It’s quite possible my gripes below can actually be solved, and it’s my lack of knowledge on Marko's deep internals that has led me to believe these things are not currently possible. So, please, take these with a grain of salt.
Dev Tools
Currently, it’s not really possible to make good dev-tools like react-dev-tools for Marko because all knowledge of custom tag names (and other valuable meta-data) is thrown away during the compile phase. In fact, the only thing that has any knowledge of the tag name, even at compile time, is the parent template which is referencing the tag. The tag name is only used to look-up the taglib and then it's discarded. This means there is no way to build a component list hierarchy in a dev-tools UI since the name is not stored as any metadata anywhere on the component itself - or anywhere in the compiled output actually. It’s lost completely. So, all you can do is create a hierarchy of the DOM elements, instead of one which references elements by their custom tag name. (see React-dev-tools for an example of the kind of Component hierarchy I am trying to create).
Transitions
Currently, the only way to do any kind of CSS effects or transitions is to do it explicitly in the component by adding an attach or detach event listener to the element and then listening for that event in the component class itself.
This forces you to hardcode transitions into a component, rather than using some kind of composition. Ideally, you should be able to listen to those events externally so things like a component for an example of what I'm refering to).
<transition>
tag can be created to wrap components with custom transitions (see something like Vue’sUnfortunately, there’s no way to externally wrap a component and listen for these events without hard-coding a ton of stuff. Even if you dispatch an event from the internal component, the wrapping component cannot catch it because the tag cannot listen to events emitted by the tag when using <include(input.renderBody) />. The only thing that seems to work is to do it in the outer wrapping component.
But this means the transition tag serves no purpose, as it’s really inside that tag you want to be able to capture and handle the event, not externally. Also, this means that the
my-custom-tag
has to explicitly listen for its own elements detachment,emit()
the detach, and then have the external component capture it. Again, you are having to do a bunch of stuff explicitly which means you cannot do it with any third party component that might not be emitting those events. Ideally what you want is something more like...Transition tag - simplified greatly to just demonstrate
Usage
But there are multiple problems here. First, you cannot listen to the attach/detach events on a component without that component explicitly listening, and then emitting those events. They don't bubble up. Second, the
<include />
tag cannot listen to events from transcluded components, so even if you did emit the events from children, it cannot capture them. This last one seems like a bug, as there are tests that seem to work if you are passing the component directly to the include tag<include(component) />
rather than doing transclusion with<include(input.renderBody) />
.Transformers
Transformers are a great addition, but the include tag again seems to bite you in the ass because there’s no way to gain access to the child component during the transform stage. When stepping through the code, it seems the transform of the tag used
<my-custom-component>
has no correlation to the actual component instance that will be created. This means if I want to intercept the custom component, all I can really do during transform is add/remove attributes to the tag, I have no access to the actual component class or it’s template to, say, add event listeners to its elements or append a function to its class (refer to the above transition problem, I was hoping I could use a transform to attach the necessary listeners to the component, but you cannot). Again, this forces me to hard code anything I want in the child component itself, rather than externally adding it to the component through composition, which would make the component more flexible.Event Listeners
Event listeners not accepting functions means you have to do a lot of boilerplate and orchestration to get events to bubble up to a parent component. For example, if I have a tablist component that takes any child component as a renderer for the items (in this case, a Tab component). If I want to react to a tab click and change the internal state of the tablist to reflect the item chosen, I have to attach a listener to the tab, then listen for that in the outer most component class, set a “clicked” state variable and then pass that back down to the tablist component, setting a selectedItem attribute, just to get the tablist to react to a tab click.
There’s currently no way to have tablist listen directly to an event from a child component. Ideally, you would want to either call a method on the parent component directly or pass in an event name to the tablist telling it what event to listen to on the child component. But, because a transcluded child cannot be listened to directly, this is not possible. Also, it's not possible to listen to an event dynamically, even if you could attach event listeners to the include tag because the event listener requires a string for the event name, not a variable that is being passed in.
Option 1 - call a method directly
Option 2 - Pass in an event name to listen for
Looking at the list above, 3 out of the 4 issues seem to relate, in some way, to how Marko works with transclusion, the include tag, and events. Unfortunately, I have no idea how difficult accomplishing these things would be. I’ve tried going through the code but the compiler + taglibs + helpers + VDOM + component library + AST + transforms + etc. is so complex, I can’t make heads or tales of where all the changes need to happen. What is probably the number 1 benefit of Marko, an awesomely advanced compiler, is also the biggest hindrance when it comes to external contributions. Without detailed docs for others to follow, it’s a huge undertaking to try and figure out where all the changes need to happen :( Compare this to React, Vue, Preact, Inferno or pretty much any other view libraries compiler, and it’s easy to see why all those libraries have so many great tools contributed by external people, they are fairly simple and easy to follow. :(
I do want to make it clear that I LOVE MARKO and I'm only writing this because I want to see it gain more popularity, but these limitations are constantly biting me every time I try to do something. It really makes creating flexible, composable, components extremely difficult. If you are only creating components for a single purpose, where hard-coding explicit event handlers, transitions, etc. are all one-offs, then Marko does a great job. But, if you are trying to build a component library, where components will be used in many different implementations, trying to externalize and compose things like transitions, tools, etc. becomes extremely difficult.