Adds actions to components #1247
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1247 +/- ##
==========================================
+ Coverage 91.88% 91.96% +0.07%
==========================================
Files 126 128 +2
Lines 4548 4602 +54
Branches 1478 1490 +12
==========================================
+ Hits 4179 4232 +53
- Misses 153 154 +1
Partials 216 216
Continue to review full report at Codecov.
|
src/generators/nodes/Element.ts
Outdated
| import Text from './Text'; | ||
| import * as namespaces from '../../utils/namespaces'; | ||
| import Behavior from './Behavior'; |
There was a problem hiding this comment.
two same imports import Behavior from './Behavior'; ^
|
Damn, @jacwright! This looks awesome. The PR is great on an initial read — just one bit of feedback, which is that generally it's better to add a Also, we could make the generated code a tiny bit simpler: -link_behavior = link(a);
+link_behavior = link(a) || {};
-if (link_behavior && typeof link_behavior.destroy === 'function') {
- link_behavior.destroy();
-}
+if (link_behavior.destroy) link_behavior.destroy();Onto the bikeshedding: one problem with 'behavior' is that's the terminology we use to describe all of a component's encapsulated logic — methods, transitions, etc. Also, it's spelt 'behaviour' 😀. Re-reading #469, I like the 'trait' suggestion for its English-language connotations, but less so for its connotations in programming. I also like <img with:lazyload data-src='potato.jpg'>
<script>
export default {
augmentations: {
lazyload(node, data) {...}
}
};
</script>Any thoughts, anyone? I also want to make sure we fully think through the lifecycle stuff, because it'd be difficult to make changes in future. Are there situations where you might want to run code before the element enters or exits the DOM? I like the simplicity of I can think of two fairly contrived examples off the top of my head — if you had CC'ing @evs-chris — Chris, do you have any thoughts on the lifecycle stuff? Any lessons from Ractive you could share? |
What do you think of one of these: "actions", "enhancements", "additions" or "extensions"? My vote goes to "actions". It is short but it has meaning, as do the others. |
|
I like I also like the simplicity of what is there with just update and destroy. We can add additional lifecycle events later if we need. The one thing that will break backwards compatibility is if we change when the initial method is run. Right now it is before mount, so I think we're ok. The default being before mount makes sense to me (many traits won't care about the DOM). What if we start with these options on the returned object:
|
|
A problem I just thought about is hydration. You can't add a trait to an element before it is in the DOM under all circumstances. If your page is server-rendered you only get the element after it is in the page. Should we account for that and just provide |
|
From my pov, am not so keen on:
Looking at your opening statement @jacwright : Also, surprised to hear @Rich-Harris actually wanting to add more events than absolutely necessary at any given time. Normally it is the other way around! |
|
I've made the changes request @Rich-Harris.
Note: elements that hydrate will not benefit from pre-mounting logic. I'm not sure if addEventListener and other DOM methods are available on the server or whether allowing traits to run on the server will cause issues. Originally my thinking was they were only client-side. |
|
Both "services" and "use" (using, uses) are interesting! |
|
@tomcon I like |
|
@tomcon's <img use:lazyload data-src='potato.jpg'>
<script>
export default {
actions: {
lazyload(node, data) {...}
}
};
</script>How does that sound for everyone?
Gah, you're absolutely right. Having a pre-mount hook is liable to cause all sorts of confusion. Better to not have it at all than to have subtle behaviour differences that people will struggle to debug. And since |
Sounds great to me! |
|
Using Getting errors on validation tests where: I cannot reproduce locally, even after a fresh |
|
Nice! I would guess the test failures are related to #1250 — |
src/validate/html/validateElement.ts
Outdated
| if (!validator.actions.has(attribute.name)) { | ||
| validator.error( | ||
| `Missing action '${attribute.name}'`, | ||
| attribute.start |
There was a problem hiding this comment.
this should now just be
validator.error(`Missing action '${attribute.name}'`, attribute)due to #1250
| if (prop.value.type !== 'ObjectExpression') { | ||
| validator.error( | ||
| `The 'actions' property must be an object literal`, | ||
| prop.start |
There was a problem hiding this comment.
see previous comment — prop, instead of prop.start
Actions add additional functionality to elements within your component's template that may be difficult to add with other mechanisms. Examples of functionality which actions makes trivial to attach are:
* tooltips
* image lazy loaders
* drag and drop functionality
Actions can be added to an element with the `use` directive.
```html
<img use:lazyload data-src="giant-photo.jpg>
```
Data may be passed to the action as an object literal (e.g. `use:b="{ setting: true }"`, a literal value (e.g. `use:b="'a string'"`), or a value or function from your component's state (e.g. `add:b="foo"` or `add:b="foo()"`).
Actions are defined in a "actions" property on your component definition.
```html
<script>
export default {
actions: {
b(node, data) {
// do something
return {
update(data) {},
destroy() {}
}
}
}
}
</script>
```
A action is a function which receives a reference to an element and optionally the data if it is added in the HTML. This function can then attach listeners or alter the element as needed. The action can optionally return an object with the methods `update(data)` and `destroy()`.
When data is added in the HTML and comes from state, the action's `update(data)` will be called if defined whenever the state is changed.
When the element is removed from the DOM `destroy()` will be called if provided, allowing for cleanup of event listeners, etc.
See sveltejs#469 for discussion around this feature and more examples of how it could be used.
|
All done. Ready to merge. |
|
Does it look like a decorator plugin in Ractive, right? |
|
@PaulMaly exactly. |
|
Tooltips, image lazy loaders, drag and drop functionality - these cases can be handled by components composition or component wrapper, do they? Lets not extend the framework with yet another syntax |
|
Your tooltip component will have to wrap your image with a span tag or
something, it can’t just add events to its children. And if you are adding
multiple actions to it you will have to wrap it multiple times.
Your LazyLoad image is now inextensible. What if you want to add a class?
Perhaps the author of LazyLoad thought of that and sets className onto the
<img>. But will the author consider everything? Perhaps if we get
{...state} attributes.
I totally get not wanting to extend the syntax. I tried doing these things
and in practice it was not easy or pretty. Actions provide a much cleaner
and easier way to accomplish a certain set of functionality that would be
much more difficult without it. It is a great tool to have at your
disposal, and one which you don’t need to use and won’t add any code to
your app if you choose not to use.
|
|
Released as 1.58.2. Thank you @jacwright! |
|
Can someone please post how to use this feature in full before it gets documented on the main website. Whats the final syntax etc. |
|
@ekhaled The syntax is pretty well described in the initial PR description, but you can also take a look at the |
|
Thanks @Conduitry. |
|
@ekhaled yeah, I updated the description to the latest decision so people didn't have to wade through the comments. I should have made a note that it was updated, sorry about that! |
|
Also, this PR should land soon too #1279 which adds some nuance to actions that ought to be included in the docs. |
|
So actions are separate modules or I should duplicate them on every component? |
|
They can be separate modules. I hope we (the community) can build up some great actions in NPM that can be used by the community. |
Looking at the discussion on #1247 it sounds like this was the intended way actions would be set up to work (which is why we didn't add a `mount` lifecycle method). I *believe* this is a fix in the original implementation. Complaints in chat about this surfaced the issue. Some libraries expect the element to be in the DOM when initializing and these libraries cannot be used without any lifecycle hook. @PaulMaly is requesting this be looked at, and I agree with his assesment. What's more, this change *should* be backwards compatable. Actions which work before this change should continue working after this change.
Actions add additional functionality to elements within your component's template that may be difficult to add with other mechanisms. Examples of functionality which actions makes trivial to attach are:
Actions can be added to an element with the
usedirective.Data may be passed to the action as an object literal (e.g.
use:b="{ setting: true }", a literal value (e.g.use:b="'a string'"), or a value or function from your component's state (e.g.add:b="foo"oradd:b="foo()").Actions are defined in a "actions" property on your component definition.
A action is a function which receives a reference to an element and optionally the data if it is added in the HTML. This function can then attach listeners or alter the element as needed. The action can optionally return an object with the methods
update(data)anddestroy().When data is added in the HTML and comes from state, the action's
update(data)will be called if defined whenever the state is changed.When the element is removed from the DOM
destroy()will be called if provided, allowing for cleanup of event listeners, etc.See #469 for discussion around this feature and more examples of how it could be used.