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

Implement app lifecycle #63

Merged
merged 30 commits into from
Sep 7, 2021
Merged

Implement app lifecycle #63

merged 30 commits into from
Sep 7, 2021

Conversation

shangzhel
Copy link
Collaborator

@shangzhel shangzhel commented Sep 5, 2021

Closes #54.

Interface changes:

  • newCard is no longer a free function, and has become a callback of IAppContext. Calling it causes a blank card to be shown in the card detail pane. The callback does not return anything (() => void) because all React state changes are asynchronous.
  • Calling update or commit on an ICard will throw unless it is the one obtained from the detail prop of Home.
  • Calling commit on a detail ICard that is not new and has not been modified will resolve but not actually send a PATCH request.
    • This should be of no concern, because nothing should happen when nothing about a card had changed.

Also included:

  • Server error handler
    • HttpStatusError is handled and the status code and message sent in response.
    • Anything else is masked with an HTTP 500 response with no detail to prevent implementation leakage.

Note:

  • Although there are code files in the controllers subdirectory, all that can be found in them are interface definitions for the controllers.
    • Concrete implementations are found in web/src/App.tsx instead, because they are coupled with user state, where the card information is actually represented, and network activity.
    • If we want to refactor it, we can, but I doubt the effort will be worth especially for this sprint.

@shangzhel shangzhel added the enhancement New feature or request label Sep 5, 2021
@shangzhel shangzhel added this to the Sprint 1 milestone Sep 5, 2021
@shangzhel shangzhel self-assigned this Sep 5, 2021
@shangzhel shangzhel added this to Sprint 1 in Product backlog via automation Sep 5, 2021
@shangzhel shangzhel added this to In progress in Sprint 1 via automation Sep 5, 2021
@shangzhel shangzhel moved this from In progress to In review in Sprint 1 Sep 5, 2021
Copy link
Owner

@chomosuke chomosuke left a comment

Choose a reason for hiding this comment

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

some random opinions about where things should be but other than that it looks good.

server/src/server.ts Show resolved Hide resolved
web/src/controllers/Card.ts Show resolved Hide resolved
web/src/controllers/Card.ts Show resolved Hide resolved
web/src/App.tsx Show resolved Hide resolved
web/src/App.tsx Show resolved Hide resolved
web/src/App.tsx Show resolved Hide resolved
@shangzhel
Copy link
Collaborator Author

Merging it now because it's blocking literally everyone else.

@shangzhel shangzhel merged commit 2c93170 into master Sep 7, 2021
Sprint 1 automation moved this from In review to Done Sep 7, 2021
Product backlog automation moved this from Sprint 1 to Sprint 1 Done Sep 7, 2021
@shangzhel shangzhel deleted the shangzhel/lifecycle branch September 7, 2021 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Product backlog
  
Sprint 1 Done
Sprint 1
  
Done
Development

Successfully merging this pull request may close these issues.

Implement app lifecycle
4 participants