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

Add guidelines for writing controllers #4110

Merged
merged 17 commits into from
Sep 2, 2024
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Mar 26, 2024

Explanation

So far we have not have any documentation on how to write controllers the BaseController v2 way. These guidelines aim to fill in the missing pieces. Note that while each guideline has examples, we are still missing a complete example controller file which demonstrates all of the guidelines in one go. That will come in a future PR.

References

Progresses #4504.

Changelog

(None)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mcmire mcmire force-pushed the add-controller-guidelines branch 2 times, most recently from 930bdba to 3764164 Compare March 26, 2024 15:32

const fooController = new FooController();

const [activeAccounts, inactiveAccounts] = select(
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'm not sure how we intend on using these selectors just for state access. I just made up this API, but not sure if reselect or something else would work as well here.

I might drop this section, though, until we've worked out how selectors would work for this use case.

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be fooController.state one line down from here, rather than fooController. We'll need to select from the state. But aside from that, this looks basically like I had imagined as well!

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 ended up updating this example to use reselect.

@mcmire mcmire force-pushed the add-controller-guidelines branch 2 times, most recently from 4412bf5 to 6ae1d55 Compare July 8, 2024 22:43
So far we have not have any documentation on how to write controllers
the BaseController v2 way. These guidelines aim to fill in the missing
pieces. Note that while each guideline has examples, we are still
missing a complete example controller file which demonstrates all of the
guidelines in one go. That will come in a future PR.
@mcmire mcmire force-pushed the add-controller-guidelines branch from 6ae1d55 to 271cbd6 Compare July 8, 2024 22:49
@mcmire mcmire marked this pull request as ready for review July 8, 2024 22:50
@mcmire mcmire requested a review from a team as a code owner July 8, 2024 22:50
@mcmire
Copy link
Contributor Author

mcmire commented Jul 8, 2024

This is ready for review again. I've expanded heavily upon the set of guidelines I posted originally, so this may need a complete re-review.

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Had some minor proofreading comments but looks great overall! Having a clear set of best practices available to reference should reduce a lot of friction and uncertainty around working with controllers.

docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Some suggestions for consistency with previous changes.

docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
docs/writing-controllers.md Outdated Show resolved Hide resolved
MajorLift

This comment was marked as duplicate.

mcmire and others added 2 commits July 9, 2024 15:07
Co-authored-by: Jongsun Suh <[email protected]>
@mcmire mcmire requested a review from a team July 16, 2024 15:19
@mikesposito mikesposito merged commit 25f460d into main Sep 2, 2024
116 checks passed
@mikesposito mikesposito deleted the add-controller-guidelines branch September 2, 2024 10:32
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.

4 participants