Skip to content

Add controllers doc #134

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

Merged
merged 22 commits into from
Mar 23, 2021
Merged

Add controllers doc #134

merged 22 commits into from
Mar 23, 2021

Conversation

justinfagnani
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Jan 29, 2021

A live preview of this PR will be available at the URL(s) below.
The latest URL will be appended to this comment on each push.
Each build usually takes around 7 minutes, and will 404 until finished.

https://pr134-8e22805---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-b615037---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-0796244---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-0c0c6c8---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-3b68076---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-79e50c2---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-f6c0b25---lit-dev-bvxw3ycs6q-uw.a.run.app/
https://pr134-0411beb---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-e422ead---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-00ba4ba---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-a7e6fb2---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-a7e6fb2---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-f65f75b---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-f65f75b---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-9c74ed9---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-b1979a1---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-b1979a1---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-efd9047---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-efd9047---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-8da5294---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-48efd54---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-48efd54---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-5d965e8---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-5d965e8---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-edd78f9---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-edd78f9---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-3aa2842---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-3aa2842---lit-dev-5ftespv5na-uc.a.run.app/
https://pr134-b4a97a5---lit-dev-5ftespv5na-uc.a.run.app/

}
```

The controller instance registers itself to receive lifecycle callbacks from the component. The component associated with a controller instance is called the host component.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this paragraph after the example and explain that the controller can trigger an update and that in this example, the updated time will be rendered periodically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the bit about triggering updates, but I'm inclined to keep the detail here instead of with the new example I added above, to keep the intro high-level.


Reactive controllers can be implemented with JavaScript classes.

A controller needs a reference to its host component so that it can register itself and interact with the component later:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A controller needs a reference to its host component so that it can register itself and interact with the component later:
A controller registers itself with its host component by calling `addController`. Typically, it stores a reference to its host component so it interact with the host. For example, to provoke an update on the host, the controller can call `requestUpdate`.

Copy link
Member

Choose a reason for hiding this comment

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

This should probably be an interactive example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added example to the top of the page

@nicolejadeyee nicolejadeyee added this to the Website complete milestone Feb 10, 2021

{% endtodo %}

#### Controller that own Directives
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
#### Controller that own Directives
#### Controllers that own directives

Copy link
Member

@sorvell sorvell left a comment

Choose a reason for hiding this comment

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

LGTM. All my comments are nits. Would like to see them addressed but happy to have this merged.

@@ -0,0 +1,181 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Why is this whole thing included? Is it because it's not published? Can we at least make a TODO to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we publish with my latest changes we can remove this.

}
}

const timeFormat = new Intl.DateTimeFormat('en-US', {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make the controller do this?


{% endtodo %}

## See Also
Copy link
Member

Choose a reason for hiding this comment

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

I dislike having a See also section at the end. It's out of context and likely the user that finally arrives here wanted this info long ago. Feel like these links should be integrated into the content where they make sense.

@justinfagnani
Copy link
Contributor Author

@arthurevans PTAL

Comment on lines +13 to +17
You can use controllers to implement features that require their own state and access to the component's lifecycle, such as:

* Handling global events like mouse events
* Managing asynchronous tasks like fetching data over the network
* Running animations
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid that to a new reader, the top examples in the bullets don't obviously connect with "features that require their own state and access to lifecycle".

Handling events and fetching data themselves don't really require state or hooking element lifecycle. It's allowing the owner to act on the results of them that requires triggering updates. Maybe it would connect better to include "triggering updates" in the description somehow, like

Suggested change
You can use controllers to implement features that require their own state and access to the component's lifecycle, such as:
* Handling global events like mouse events
* Managing asynchronous tasks like fetching data over the network
* Running animations
You can use controllers to implement features that require their own state and access to the component's lifecycle, such as:
* Triggering component updates based on global event listeners like mouse events
* Managing asynchronous tasks like fetching data over the network and triggering updates when tasks complete
* Running animations

...or something (not sure that's great). But just saying "handling events" doesn't make the connection I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

#2 would be simpler in the singular, like "Managing an asynchronous task like fetching data over the network, and triggering an update when the task completes."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point in (1) is not triggering the update necessarily - you can already do that with el.requestUpdate() - but using the hostConnected and hostDisconnected callbacks to add and remove the listeners.

Similarly with tasks, a key feature of controller is having the hostUpdate hook so you can pull new deps.


You can also create controllers that are specific to `HTMLElement`, `ReactiveElement`, `LitElement` and require more of those APIs; or even controllers that are tied to a specific element class or other interface.

`LitElement` and `ReactiveElement` are controller hosts, but hosts can also be other objects like base classes from other web components libraries, components from frameworks, or other controllers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`LitElement` and `ReactiveElement` are controller hosts, but hosts can also be other objects like base classes from other web components libraries, components from frameworks, or other controllers.
`LitElement` and `ReactiveElement` are controller hosts, but hosts can be any object that implements the [ReactiveControllerHost](/guide/api/controllers/#ReactiveControllerHost) interface described above; these could include base classes from other web components libraries, components from frameworks, or other controllers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like name & link should occur up above, rather than down here?

}

constructor(host: ReactiveControllerHost) {
(this.host = host).addController(this);
Copy link
Member

Choose a reason for hiding this comment

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

For understanding, the addController here isn't really required right? This is more of a "controller wrapper" than "controller proper", since it doesn't implement any callbacks itself. Not sure that makes it a bad example per se, but having this addController here sort of makes it unclear what's going on since it's not actually required and wouldn't need the implements ReactiveController.

But it begs the question, Is a controller a controller if it only calls requestUpdate and doesn't implement any of the ReactiveController interface...? Seems like by this example's definition, a controller is either an object that implements ReactiveController interface, or else somehow gets and doinks with a ReactiveControllerHost, or both.

Sorta makes a controller harder to define.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be clearer if you removed the addController here and added a comment that this is basically a wrapper that configures the task controller and exposes a render() method to the host component...

Or it could act as a controller, and delegate the callbacks to task. Probably less efficient that way, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the addController() call, but yeah, controller are a bit hard to defined because they're helpers and can use any amount of API on the host. If we try to limit the use of "controller" to object passed to addController() we're going to have to figure out what to call these other things.

Copy link
Contributor

@arthurevans arthurevans left a comment

Choose a reason for hiding this comment

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

Left some comments, basically looking good. Need to decide whether we're finishing the sections on controller directives & animations for MVP.

Tasks example feels weird and complicated. Maybe it just needs a little more explanation or comments on the main controller?

Comment on lines +13 to +17
You can use controllers to implement features that require their own state and access to the component's lifecycle, such as:

* Handling global events like mouse events
* Managing asynchronous tasks like fetching data over the network
* Running animations
Copy link
Contributor

Choose a reason for hiding this comment

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

#2 would be simpler in the singular, like "Managing an asynchronous task like fetching data over the network, and triggering an update when the task completes."


You can also create controllers that are specific to `HTMLElement`, `ReactiveElement`, `LitElement` and require more of those APIs; or even controllers that are tied to a specific element class or other interface.

`LitElement` and `ReactiveElement` are controller hosts, but hosts can also be other objects like base classes from other web components libraries, components from frameworks, or other controllers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like name & link should occur up above, rather than down here?


`@lit-labs/task` contains a generic `Task` controller that can pull inputs from the host, execute a task function, and render different templates depending on the task state.

You can use `Task` to create a custom controller with an API tailored for your specific task. Here we wrap `Task` in a `ForexController` that can fetch foreign exchange data from a REST API. `ForexController` exposes a `currency` property as an input, and a `render()` method that can render one of four templates depending on the task state. The task logic, and how it updates the host, are abstracted from the host component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this example too complicated? I had trouble following it, and the main controller, as Kevin noted, it's not obvious why it needs to be a controller. Also the task.ts has a ton of "TODO" type comments in it, which feels a little weird in an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have task.ts updated in @lit-labs/task. Will TODO to switch to that and remove it from the demo

justinfagnani and others added 11 commits March 22, 2021 11:57
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
Co-authored-by: Arthur Evans <arthure@google.com>
@justinfagnani justinfagnani merged commit 2e65bd3 into master Mar 23, 2021
@justinfagnani justinfagnani deleted the controllers branch March 23, 2021 17:39
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.

5 participants