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

feat(shared-types): move billing related types to shared folder #2400

Merged
merged 4 commits into from
Aug 2, 2021

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Jul 15, 2021

Note that this PR builds on the typings exposed by #2385

Problem

This PR extracts out billing related types to the root shared folder

Related to #2066

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible
    • Only type changes

Features:

  • move billing related types to root shared folder
  • use relocated types in client and server

@karrui karrui changed the title ref(shared-types): move billing related types to shared folder feat(shared-types): move billing related types to shared folder Jul 15, 2021
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

I find the new organisation a little strange now (i.e. login depending on billing to extend some of the types). Why would LoginBase be in billing for example? 🤔

I'd think login is a "more fundamental" blocks other modules depend on.

Similar comment for LoginStatistic being a copy of FormBillingStatistic

I wonder if this indicates there should be a types/login.ts in the shared folder instead?

import { IUserSchema } from './user'

export interface ILogin {
export interface ILogin extends LoginBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does ILogin still need to be exported? Looking at the rest of the codebase, it looks like the expectation is to only use ILoginSchema? (I only see ILogin being used in test, not in app code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can probably stop exporting that; seems like the newer mongoose versions automatically type with a lean version of xxxSchema types when creating a new document. It used to be the case that we needed a separate typing for creating documents, as xxxSchema types contain extra props that is not needed to create a new document.

formId: IFormSchema['_id']
authType: ILoginSchema['authType']
export interface ILoginSchema extends ILogin, Document {
created?: Date
Copy link
Contributor

Choose a reason for hiding this comment

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

By moving created to ILoginSchema, is it still optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since this typing is used for document creation, and we don't want to enforce a created prop to be assigned for document creation.

We should actually create a LoginDocument typing for the object that is retrieved from the database (with all the defaults populated, and have the optional types converted to required), but there are currently no cases where that is needed and thus not done yet.

@karrui karrui force-pushed the feat/dto-billing branch 2 times, most recently from 5254c6e to e40c635 Compare July 26, 2021 11:08
Base automatically changed from feat/dto-form to develop July 27, 2021 02:28
@karrui karrui force-pushed the feat/dto-billing branch from e40c635 to 44cd711 Compare July 27, 2021 02:30
@karrui
Copy link
Contributor Author

karrui commented Jul 27, 2021

I find the new organisation a little strange now (i.e. login depending on billing to extend some of the types). Why would LoginBase be in billing for example? 🤔

I'd think login is a "more fundamental" blocks other modules depend on.

Similar comment for LoginStatistic being a copy of FormBillingStatistic

I wonder if this indicates there should be a types/login.ts in the shared folder instead?

Login is kind of a misnomer (but immortalized as a collection in the database 😢 ) since it's actually tracking Corppass/Singpass logins for billing purposes, and since the types are to be shared I thought to make it clearer. Should I change it back to login?

@karrui karrui requested a review from timotheeg August 2, 2021 02:00
Copy link
Contributor

@timotheeg timotheeg left a comment

Choose a reason for hiding this comment

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

Sorry I didn't mean to be blocking this PR 😱

I think with the comment in billing.ts, it's probably good enough as is. And we can reorg again later if needed.

Let's go!

@karrui karrui merged commit 2726ee6 into develop Aug 2, 2021
@karrui karrui deleted the feat/dto-billing branch August 2, 2021 06:21
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.

3 participants