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.

Sign up for GitHub

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

Jump to bottom

Redirect API decoupling #71

Merged
merged 1 commit into from
May 14, 2020
Merged

Redirect API decoupling #71

merged 1 commit into from
May 14, 2020

Conversation

JasonChong96
Copy link
Contributor

Problem

The redirect API and model components are highly coupled. This makes it difficult to inject mocks for automated user testing. Prerequisite for solving #70

Solution

Decouple redirect API from Redis and Sequelize using InversifyJS. Split up code in the model component.

Improvements:

  • redirect.ts has no knowledge of cache and database implementation
  • index.ts in server/model has been split into user.ts and url.ts

Deploy Notes

New dependencies:

  • inversify: Dependency injection library
  • reflect-metadata: Allow the use of annotations in js/ts

@JasonChong96 JasonChong96 changed the title Dependency injection Redirect API decoupling May 12, 2020
@JasonChong96 JasonChong96 requested a review from yong-jie May 12, 2020 07:22
yong-jie
yong-jie reviewed May 12, 2020
View reviewed changes
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

Gonna take some time to review the PR, but i have a preliminary comment that you can address right away while i'm at it, and that's the weird code formatting that appears in various places throughout this branch's changelog

src/server/models/url.ts Outdated Show resolved Hide resolved
yong-jie
yong-jie requested changes May 12, 2020
View reviewed changes
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

Reading up on reflect metadata and inversify, will review the logic of it shortly, but there's some comments that we can begin discussing here

src/types/server/api/analytics.d.ts Outdated Show resolved Hide resolved
src/types/server/api/analytics.d.ts Outdated Show resolved Hide resolved
src/server/inversify.config.ts Outdated Show resolved Hide resolved
src/server/api/repositories/url.ts Outdated Show resolved Hide resolved
src/server/api/cache/url.ts Outdated Show resolved Hide resolved
@JasonChong96
Copy link
Contributor Author

Pushed a new commit to address changes

@JasonChong96 JasonChong96 force-pushed the dependency-injection branch 4 times, most recently from 96c5cc7 to d377c50 Compare May 14, 2020 03:37
@JasonChong96 JasonChong96 force-pushed the dependency-injection branch from d377c50 to cb8956a Compare May 14, 2020 03:56
@JasonChong96 JasonChong96 requested a review from yong-jie May 14, 2020 03:59
yong-jie
yong-jie approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@yong-jie
Copy link
Member

Remember to use the squash and merge option

@JasonChong96 JasonChong96 merged commit 85c206c into develop May 14, 2020
@JasonChong96 JasonChong96 deleted the dependency-injection branch May 15, 2020 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liangyuanruo liangyuanruo liangyuanruo left review comments

@yong-jie yong-jie yong-jie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JasonChong96 @yong-jie @liangyuanruo