Updating my node version to v16, I noticed many tests breaking, because
the way errors are formatted did change. This commit ensures that the
test suite keeps working for all node versions.
The previous changes in the combine method (used to copy all the
variables defined in the current scope for use in a slot) had the effect
of squashing the prototype chain: instead of `Component -> Obj1 ->
Obj2 -> Obj3`, the combined scope had: `Component -> Obj1'`.
This has an unfortunate interaction with the way t-call is implemented,
which uses the fact that we are in a subscope to add a own
__access_mode__. It depends specifially on the prototype chain, and that
the parent scope may have a different value for that property. But with
the way combine was implemented, we lost all that subtlety since
everything is squashed.
In this commit, we reimplement that function in a way to make sure we
keep the prototype chain structure
In some situations (a slot inside a slot), the combine method was
wrongly copying all properties of the scope into the context, which
caused the event handling system to wrongly use a subobject as component
(since it detects the fact that __owl__ is a own property_).
Consequently, we could have very subtle issue with some properties being
shadowed by a sub object.
Before this commit, Owl could not detect that the underlying component
in a template such as <t t-component="{{state.child}}"/> was changing,
if the two components have the same tag as root element.
This is because the reconciliation is done at the vdom level, which does
not know about components. To solve this, one could use a t-key to make
sure owl can make the difference.
With this commit, we can simply use our knowledge of the fact that we are
dealing with a dynamic component and autogenerate a suitable key.
closes#623
the low level method htmlelement.classList.add does not accept multiple
classes in one string, which is why, in owl, the expression
`<div t-att-class="{'a b c': value}" />`
did not work as one might expect. It is however very convenient in real
life templates, so this commit improve owl by adding support for this
feature.
closes#813
Before this commit, the error handling code simply destroyed the
application whenever an unhandled error occured in the owl rendering
process. This is perfectly fine, except that since the application is
potentially corrupted, the destroy code may crash as well. We simply
catch those errors to avoid shadowing the main issue.
closes#866
Before this commit, owl was erroneously defining default slots in most
cases, even though they are empty. The problem occurs when the content
of a component slots is a t-set-slot, and we remove that, then use the
rest as default slots, even though it is only reduced to a set of text
nodes.
With this commit, we only consider the content as default content if it
is not only a sequence of text nodes containing only spaces.
closes#882
The templates contained in a slot have to be rendered with the current
scope, but during the rendering of the sub component, which happens
later (after the willStart for the sub component). Therefore, we need to
save the scope that should be used for the slots, so we can access the
proper variables. This was done using an Object.assign() statement,
which is actually only a shallowclone for the own properties.
In this commit, we properly copy all the properties for the current
scope, even those contained in the prototype.
closes#855
Before this commit, props and t-props were computed like
`Object.assign(props, t_props)`.
Now, it computes like `Object.assign(t_props, props)` so props will
override t-props.
closes https://github.com/odoo/owl/issues/886
Before this commit, the error handling process was too naive: once an
error occurs in a rendering, owl catches it, looks for a component that
implements the catchError method, then calls it.
However, in real life, we sometimes need to rethrow that error (or
another one) to propagate the error to some parent handler. This error
needs to be handled by the closest parent component that implements
catchError.
This is what this commit implements: it wraps the catchError call in a
try/catch, then in case of errors, try to handle it by a parent.
Have a t-call within a t-set-slot of a component.
The called template has a t-component directive.
It should be like:
```xml
<t t-name="Zero">
<Slotted>
<t t-call="someTemplate" />
</Slotted>
</t>
<t t-name="someTemplate">
<SomeComponent />
</t>
```
Before this commit, the parent of SomeComponent was the Zero component
After this commit, the parent of SomeComponent is the Slotted Component as it should be
closes#862
Previously, the order of resolution was:
- components defined on the class of the current component
- components defined on the QWeb instance
- the current instance's context
This is unnatural because it doesn't go from most specific to least, as
the current instance's context is the most specific. This is also
fragile, as adding components to the QWeb instance can break unrelated
components.
This commit fixes that by making the lookup start with the current
component's context instead
The initial problem solved by this commit is that it was possible to get
into a situation where a mounting/rendering was started, then the component was
updated, but then another mounting operation begins, and it tries to
reuse the previous rendering operation, which is no longer uptodate.
The underlying issue is that Owl did not track properly the various
internal state change of a component. These issue should be solved by
the introduction of the status enum, which currently tracks 6 possible
states:
- CREATED
- WILLSTARTED
- RENDERED
- MOUNTED
- UNMOUNTED
- DESTROYED
This status number replaces the isMounted and isDestroyed boolean flags.
It has the advantage of making sure that the component is in a
consistent state (it is no longer possible to be destroyed and mounted,
for example)
Another advantage is that it gives us an easy way to track the fact that
a component has been rendered, but is not in the DOM. This is a subtle
situation where some various events can happen, and we need to be able
to react to that case.
Note that there is a change of behaviour: if a component is mounted in a
specific target, then before the mounting is complete, the component is
mounted in another target, we no longer reject the first mounting
operation.
From the beginning, Owl was designed to only call event handler on
components that are mounted in the DOM. The main benefit is that if a
component is destroyed, we are guaranteed to not execute any useless (or
potentially dangerous code).
However, there is one downside: if a component tree is being mounted,
and a child component trigger an event in its mounted hook, then it
cannot be caught by the parent, since the parent is technically not yet
mounted.
This may not be a good situation, but the point is that Owl
unnecessarily prevent the handler to be called.
We can fix this issue by simply checking if the component is not destroyed
instead of checking that it is mounted.
closes#809
This commit makes sure that Owl does not crash when a component is
created, then updated (for example, with a (observed) state change), and
then, some moment later, mounted.
The initial render is not useful, because it is not linked to a mounting
action anyway. And it caused issues such as a crash when Owl tried to
patch it to a non existing target
This commit tries to improve the interactions involving unmounted
components, or components mounted in an htmelement which is detached
from the main DOM, and rendering actions.
The main example is mounting a component in detached div, to prepare all
children. If we just mount the component, it will work as expected: the
full component tree is rendered in memory, and ready to be really
mounted at the desired target.
However, if before doing that, we update the component and call render
on it (for example, with a change in an observed state), then this
rendering will be ignored, and therefore, the full subcomponent tree is
not uptodate.
This commit will also solve another issue in the compatibility layer in
odoo: in the form renderer, we mount components with the adapter in a
div, which is not yet attached to the DOM. We then manually call the
mounted hook when on_attach_callback is called. This means that before
this commit, any change to the components between the initial rendering
and the call to mounted will be ignored.
As a bonus, this commit has the effect of bringing closer the semantics
of render and mount operations, which is certainly good.
closes#823
In some cases, a rendering initiated in some component is then remapped
into a larger rendering initiated by some parent.
If we have some components which implement shouldUpdate to return false,
then the following scenario can happen:
- some parent component is mounted (which triggers a rendering with
force: true => bypass the shouldUpdate)
- some sub component is updated and rerendered, AFTER the previous
rendering goes through it
- the sub component notices that there is an ongoing rendering, and
remaps itself in the parent rendering
Before this commit, the new fiber in the subcomponent does not have
force flag set to true, so the new rendering for the subcomponent does
not go through its own children (if they have shouldUpdate=false)
Another more complex kind of scenaria can happen when a remapped
rendering happen with sub components with dynamic shouldUpdate. The
problem is the same at the end: the new rendering should ignore the
shouldUpdate, to make sure we have the last correct information.
With this commit, we make sure that the flag of the new fiber is set to
true.
closes#818
Components can implement shouldUpdate to return false. In that case,
renderings coming from above should be ignored.
However, if the component was unmounted and is remounted, we actually
need to force a rerendering in that case, so it is mounted, otherwise
the subcomponent is left in unmounted state, which means that rendering
are ignored.
closes#800
The reviewer for the commit on named slots inside named slots did not
notice that there was an infinite loop. Because of his sloppiness, Owl
could block in an infinite loop when a named t-slots was defined inside
a subcomponent, but not as a direct child.
Strangely, the _compile method required a CompilationContext whenever it
was compiled as a sub template, but this parent context was actually not
really needed. I guess that it was the case in the past, but this was
changed at some point.
This commit makes another significant change: the xml element is no
longer mandatory. It is actually only required for slots (because the
template is not registered to qweb).
Finally, the interface for the whole method has been changed to use an
option object, which makes more sense with 3 optional paremeters.
Previous code naively handled nested t-set-slots: if a second named
slots was found, it overrode the first.
In this commit, we use a set to make sure that we only use the first
found t-set-slot node. Also, we ignore set-slots defined in a sub
components, because these slots are only relevant to the sub component
itself.
Note that it works as expected because document.querySelectorAll
performs a search depth first, so we will always use the named slots
closer to the parent element, in term of depth.
closes#682
Before this commit, the t-model directive worked well with expressions
such as "state.value", but not with bracketed expression: "state[value]"
(it generated invalid code).
This commit make the t-model smarter by detecting this case, and
properly capturing the base expression and key variable.
closes#694
This is a rarely (if ever) used feature, but according to our qweb
reference implementation, it is possible to use the
t-call directive on an arbitrary html tag, like this:
<div t-call="my.template"/>
It is then interpreted as:
<div><t t-call="my.template"/></div>
So, with this commit, we make sure that the owl qweb implementation
matches that behaviour.
closes#706
The previous fix (overriding tostring of VDomArray) is actually more
general, and solves the same issue. So, let us simplify the code and
keep the more general solution.
This reverts commit 3bf91afc3f.
Consider this scenario:
- a variable v (with a body) is defined in a template
- it is then passed to a sub component as a prop
- and now, it is t-esc-ed.
Before this commit, the displayed value was [object object], because the
value actually passed to the sub component was a VDomArray (internal
structure used to represent nodelists)
This issue is actually quite a problem in practice, because values in a
templates are translated, but not in attributes. Therefore, using a
t-set directive with a body text content is the proper way to have
translated values at runtime.
We override in this commit the method toString of VDomArray to make sure
it is properly displayed.
Note that we considered changing the way props were generated (by trying
to detect VDomArray, then calling vDomToString), but then the value
would not be able to be used in a t-raw. Also, it is quite elegant to
be able to format the VDomArray only at the end.
closes#670
Because of a "break" statement instead of "continue", the check for valid
props was stopping as soon as it met an optional props, which kind of
invalidate the whole system.
closes#717
Sometimes, HTML is slightly more subtle than what I initially expect.
Rendering some html is simple, we have tags and attributes. However,
once we add behaviour, then the situation is more complex:
<input value="abc"/>
is an input with an INITIAL value of "abc", but the attribute does not
actually represent the CURRENT value of the input, which may be
different if the user did change it.
This is basically the difference between "attribute" and "property".
So, when rendering html with owl, we sometimes want to actually set
the property (current value), instead of the html attribute.
This commit make sure that this is the case for inputs with the "value"
attribute.
closes#722
Here is a situation that can happen in some complicated case:
1. a parent component is rendered, which includes some children
2. it is then willPatched
3. the sub components are then mounted/willUnmounted
4. because of complicated business logic, this causes the parent
component to be rerendered (before parent "patched" method is called)
5. owl will internally reset its currentfiber to null (but there is a
pending rendering!)
6. subsequent rendering will ignore pending rendering
7. havoc ensues
This is actually one of the reason why modifying a component state in a
willPatch component is actually not a good idea. However, the good news
is that this specific situation can be properly handled: we can simply
make sure that we do not reset currentFiber to null if there is a new
pending rendering.
closes#728
Owl has to manage a lot of interesting situations. One of them is when
a rendering is initiated, which creates a sub component, but then
another rendering starts, which invalidate the previous one, and will
create another sub component. Since the first sub component was not
ever in the DOM, we cannot rely on the vdom patching process to remove
it, so we have to do it manually.
Sadly, this is actually a very tricky situation, since there are other
subtle situations where the code that remove an unmounted widget could
be executed, in particular when the parent component is unmounted, then
remounted, then modified to trigger yet another rendering.
In this commit, we handle this case more carefully by making sure that
the destroyed subcomponent properly configures its pvnode so the patch
process happens as expected.
joint work with the framework team, and in particular LPE for his work on
finding a testcase!
closes#724, #731
Have a hierarchy of A, B, C components where:
```xml
<div t-name="A">
<div>
<B t-key="key1"/>
</div>
</div>
<t t-name="B">
<C t-key="key2"/>
</t>
<div t-name="C">
<div><t t-esc="keys_as_props" /></div>
</div>
```
The subtility of the issues lies in B, which doesn't have its own
concrete DOM element, rather, it borrows it from C.
With the sequence of events:
- change key2
C1 is destroyed and replaced by another instance, and another node.
B1 has its props updated and is patched with the C2's node (CRITICAL)
A1 is patched
- change key1 AND key2
C2 is destroyed
B1 is destroyed
A1 is patched replacing B1 by B2, and their nodes too (which at this point should be C2's to C3's)
Before this commit, at the CRITICAL point, the node representing the component itself
(technically its pvnode) was not updated with the new concrete node provided by B1 patch with C2 node
i.e. it held the previous node still
The second array of steps crashed because at A1 patch, the new B2 node would replace B1, which
was out of the DOM (removed because C1 was destroyed long before),
and therefore without a viable parent to insert B2 node.
After this commit, we update the component's pvnode after the patch which elm had possibly changed
There is no crash anymore for this use case.
Have a t-call nested in a t-foreach nested in a t-foreach
```xml
<t t-name="template">
<t t-foreach="..." t-as="a">
<t t-foreach="..." t-as="b">
<t-call="templateCalled" />
</t>
</t>
</t>
```
Before this commit, the `a` variable was not accessible within the t-call.
That was because the way t-call protected its scope by hiding other protected scope
in this case, the first protected scope for the first `t-foreach` was hidden
After this commit, `a` and `b` are accessible in the t-call, whether the t-call
defines its own variables by `t-set` or not.
Also, as expected from other fixes, there is no leaks of variables defined within a `t-call`
fixes#695
With QWeb, we can register globally templates (using the xml tag or
the registerTemplate function). However, these templates, once
compiled, can generate sub template compiled functions. Before this
commit, these sub functions were local to a specific instance.
This means that creating a new QWeb instance and rendering a global
parent template would crash, since it was unable to find the actual sub
function.
This commit fixes the issue: the sub functions are now shared
statically, but with a unique ID, so we do not have issues with sub
functions having a same name in different QWeb instance.
closes#701
Before this commit, Env was an indexed type, this means that one could
write env.anything, and it would accept it as a valid type. This is
actually quite dangerous, because we lose the typing advantages for all
keys that are properly defined.
For example, if a component is defined as:
class MyComponent extends Component<Props> {
...
}
Then Typescript will let it use anything from the environment, even if
it is wrong. So, most properly typed Typescript applications should use
instead a sub environment:
interface MyAppEnv extends Env {
someKey: someValue
}
Then, the component should be defined this way:
class MyComponent extends Component<Props, MyAppEnv> {
...
}
Before this commit, any typos in the environment accesses would not be
noticed by typescript.
It is possible that a Component is extended dynamically and if this is
the case, the class that extends it can be anonymous, with property
name=''. If this is the case, current implementation interprets the empty
string to be false so the while loop is terminated without further
scanning the super classes.
In this proposal, we allow anonymous class to be scanned until its
Component ancestor. Basically, the anonymous class assumes the name of
it super.
This new t-set-slot directive is meant to replace t-set when we need to
define the content of a sub slot. All new code should use that
directive.
The old t-set directive is still supported for now, but this should be
removed when we publish Owl 2.0.
Unfortunately, we chose to use the directive `t-set` to define sub slot
contents in a template.
The goal was to reuse a directive for a similar use case: defining sub
template is almost the same as defining a slot content.
Obviously, this introduces a name conflict: the inner content of a
component cannot use t-set t-value anymore (nor t-set with a body
value), since they are interpreted as slot names.
We mitigate the issue here by only interpreting as slot content the
`t-set` statement located immediately below the parent component tag
name and with a body content.
However, a real fix need to introduce an additional directive to resolve
the ambiguity.
Have something like
```xml
<div t-name="Parent">
<t t-call="nodeTemplate">
<t t-set="recursive_idx" t-value="1"/>
<t t-set="node" t-value="root"/>
</t>
</div>
<div t-name="nodeTemplate">
<t t-set="recursive_idx" t-value="recursive_idx + 1"/>
<p><t t-esc="node.val"/> <t t-esc="recursive_idx"/></p>
<t t-foreach="node.children or []" t-as="subtree">
<t t-call="nodeTemplate">
<t t-set="node" t-value="subtree"/>
</t>
</t>
</div>
```
Where we want to propagate a recursion index through recursive t-calls
Before this commit, it did not work as we protected the scope in order
to not leak, in the wronf manner. Namely the protected scope only took
firt level prototype properties of the original scope.
After this commit, this case works as we mark the scope as read only
solves #672
Have
```xml
<body t-name="webclient" />
```
and
```js
const comp = new WebClient();
comp.mount(document.body, {position: 'self'});
```
Before this commit, the body that was there before anything had happened
was *replaced* by the new body node created by the WebClient OWL component
After this commit, we ensure that the element body is the same at reference level
Since ee956a197, the rendering is skipped if the currentFiber is
completed. Unfortunately, cancelled fibers remain set in __owl__,
so when a fiber is cancelled, subsequent calls to render are
skipped.
It would be nice to reset __owl__.currentFiber to null when the
fiber is cancelled, but when trying to do so, a lot of tests fail.
Part of issue #622Closes#665
In this commit, we uses the "!" as suffix to designate an event that
should be captured. This is inspired by the Vue source code.
This solves the issue of communicating additional information to the
underlying virtual dom. However, this will most likely disappear when
we rewrite the vdom as a virtual block system.
closes#650
Before this commit, creating a sub component with
t-att-style/t-att-class attributes or with t-on- event handlers would
override the *hook* object rendered by the sub component.
This is an issue for some directives, such as t-ref, which defines
hook functions.
This commit fixes the issue by checking for an override, and wrapping
the hooks in a function that calls each defined hook.
closes#638
Géry Debongnie