Skip to content

Use composition instead of classes with Layout and LifeCycleHook #412

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

Closed
rmorshea opened this issue Jun 24, 2021 · 1 comment
Closed

Use composition instead of classes with Layout and LifeCycleHook #412

rmorshea opened this issue Jun 24, 2021 · 1 comment

Comments

@rmorshea
Copy link
Collaborator

This is similar to #324 but specifically focused on the Layout and LifeCycleHook since those will be the most complex to refactor. I'm not really sure what the interface would ultimately look like - whether there would still be a layout with render and dispatch methods, or if those would be functions accepting some sort of LayoutState instead. Maybe a bit like:

with layout_context(component) as layout:
    old_state, new_state = await render(layout)
    diff = compute_diff(old_state, new_state)
    await dispatch_event(layout)

You could take a similar approach to hooks as well, but just applying functions to objects containing state.

rmorshea added a commit that referenced this issue Jul 8, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.
rmorshea added a commit that referenced this issue Jul 8, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.
rmorshea added a commit that referenced this issue Jul 9, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.
rmorshea added a commit that referenced this issue Jul 9, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 9, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 9, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 12, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 12, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 13, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
rmorshea added a commit that referenced this issue Jul 13, 2021
The `Layout` is now a prototype, and `Layout.update` is no longer a
public API. This is combined with a much more significant refactor of
the underlying rendering logic.

The biggest issue that has been resolved relates to the relationship
between `LifeCycleHook` and `Layout`. Previously, the `LifeCycleHook`
accepted a layout instance in its constructor and called
`Layout.update`. Additionally, the `Layout` would manipulate the
`LifeCycleHook.component` attribute whenever the component instance
changed after a render. The former behavior leads to a non-linear
code path that's a touch to follow.  The latter behavior is the most
egregious design issue since there's absolutely no local indication
that the component instance can be swapped out (not even a comment).

The new refactor no longer binds component or layout instances to a
`LifeCycleHook`. Instead, the hook simply receives an unparametrized
callback that can be triggered to schedule a render. While some error
logs lose clarity (since we can't say what component caused them). This
change precludes a need for the layout to ever mutate the hook.

To accomodate this change, the internal representation of the layout's
state had to change. Previsouly, a class-based approach was take, where
methods of the state-holding classes were meant to handle all use
cases. Now we rely much more heavily on very simple (and mostly static)
data structures that have purpose built constructor functions that much
more narrowly address each use case.

After these refactors, `ComponentTypes` no longer needs a unique
`id` attribute. Instead, a unique ID is generated internally which
is associated with the `LifeCycleState`, not component instances
since they are inherently transient.
@rmorshea
Copy link
Collaborator Author

rmorshea commented Jul 16, 2021

This is complete. The drastic changes suggested in the original comment aren't really productive because there's not much use in having access to the internal state of Layout. The ability to substitute Layout for some other implementation is what is most useful.

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

No branches or pull requests

1 participant