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

Master doc ged #1137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Master doc ged #1137

wants to merge 1 commit into from

Conversation

ged-odoo
Copy link
Contributor

No description provided.

@ged-odoo ged-odoo force-pushed the master-doc-ged branch 2 times, most recently from 1a5da0c to daa58d4 Compare March 1, 2022 14:49
Copy link
Contributor

@poma-odoo poma-odoo left a comment

Choose a reason for hiding this comment

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

This is a very useful and interesting document, I hope you will have time to complete and publish it soon, it is a pity that it had to be postponed for such a long time. Please inform me if I can help in any way.

## Reactivity

With the reactivity system, each value created with the `useState` or the `reactive`
function is a proxy that allows Owl to trask which keys/values have been read
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function is a proxy that allows Owl to trask which keys/values have been read
function is a proxy that allows Owl to track which keys/values have been read

Comment on lines +83 to +84
that some => may miss
rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
that some => may miss
rendering
that some may miss rendering.
( Don't `let a=toRaw(r); f(a);` do `f(toRaw(r));` )

```

```js
// bad => will cause extra rendering, possibly corrupt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is bad?

});
```

other note: minimize async stuff
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an orphan in this context

Comment on lines +150 to +155
A

/ \
B C
/ | \ | \
D E F G H
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A
/ \
B C
/ | \ | \
D E F G H
```mermaid
graph TD
A-->B
A-->C
B-->D
B-->E
B-->F
C-->G
C-->H
```

Notes:

- model could be created elsewhere, in a service, in the start code, ...
- one may want to mark some internal stuff as 'raw'...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- one may want to mark some internal stuff as 'raw'...
- one may want to mark some internal stuff as 'raw', when reactivity is not necessary, to save the resources.


- model could be created elsewhere, in a service, in the start code, ...
- one may want to mark some internal stuff as 'raw'...
- if created elsewhere, it should probably be done with `reactive`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- if created elsewhere, it should probably be done with `reactive`
- if created elsewhere, it should probably be done with `reactive` instead of `useState`, e.g. `obj.model = reactive(new MyModel)`

- model could be created elsewhere, in a service, in the start code, ...
- one may want to mark some internal stuff as 'raw'...
- if created elsewhere, it should probably be done with `reactive`
- the call to `reactive` could be done in the model constructor directly:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- the call to `reactive` could be done in the model constructor directly:
- the call to `reactive` could be done in the model constructor directly, so there would be no need to call `reactive` on each use:

class MyModel {
constructor() {
...
return reactive(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return reactive(this);
return reactive(this); // All instances of MyModel will be recative automatically

Comment on lines +310 to +326
- one could slightly simplify the model use with a hook:

```js
function useModel() {
const env = useEnv();
const model = env.model;
return useState(model);
}
```

This can be simply used like this in a child component:

```js
setup() {
this.model = useModel();
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost a duplicate of line 222

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.

2 participants