kevinschaich / mintable

🍃 Automate your personal finances – for free, with no ads, and no data collection.
https://kevinschaich.io/mintable/
MIT License
1.52k stars 200 forks source link

Migrate to TypeScript #27

Closed kevinschaich closed 4 years ago

kevinschaich commented 5 years ago

Lots of errors could be resolved with some nicer types, i.e. TypeScript.

kevinschaich commented 4 years ago

@blimmer – wanted to bounce this off you as you've been a very helpful thought partner and have a vested interest in #46 longer-term. Was thinking about something like the following for the v2.0.0 configuration API:

// Integrations

enum IntegrationType {
  Import = 'import',
  Export = 'export'
}

enum IntegrationId {
  Plaid = 'plaid',
  Google = 'google'
}

interface BaseIntegrationConfig {
  id: IntegrationId
  name: string
  type: IntegrationType
}

enum PlaidEnvironmentType {
  Development = 'development',
  Sandbox = 'sandbox'
}

interface PlaidConfig extends BaseIntegrationConfig {
  id: IntegrationId.Plaid
  type: IntegrationType.Import

  environment: PlaidEnvironmentType

  clientId: string
  secret: string
  publicKey: string
}

interface GoogleTemplateSheetSettings {
  documentId: string
  sheetId: string
}

interface GoogleConfig extends BaseIntegrationConfig {
  id: IntegrationId.Google
  type: IntegrationType.Export
  oauthUrl: string

  template?: GoogleTemplateSheetSettings

  clientId: string
  clientSecret: string

  refreshToken?: string
  scope?: string
  tokenType?: string
  expiryDate?: string
}

type IntegrationConfig = PlaidConfig | GoogleConfig

// Accounts

interface BaseAccountConfig {
  name: string
  integration: IntegrationId
}

interface PlaidAccountConfig extends BaseAccountConfig {
  token: string
}

type AccountConfig = PlaidAccountConfig

// Properties

enum PropertyType {
  Automated = 'automated',
  Manual = 'manual'
}

interface Property {
  id: string
  name: string
  type: PropertyType
}

interface BalanceProperty extends Property {}

interface TransactionPropertyOverride {
  sourcePropertyId: string // the other property to test on (e.g. "Title")
  match: RegExp // a regex to match (e.g. "*(Wegman's|Publix|Safeway)*")
  replace: RegExp // a regex to replace (e.g. "Grocery Stores")
  flags?: string // regex flags (e.g. "i" for case insensitivity)
}

interface TransactionProperty extends Property {
  overrides: TransactionPropertyOverride // override default values
}

// Balances

interface BalanceConfig {
  enabled: boolean
  properties: PropertyType[]
}

// Transactions

interface TransactionConfig {
  enabled: boolean
  properties: PropertyType[]
}

// Wrapping it all together:

interface Config {
  integrations: IntegrationConfig[]
  accounts: AccountConfig[]
  balances: BalanceConfig[]
  transactions: TransactionConfig[]
  debugMode: boolean
}

Hit me with your feedback here. Hope this will make things a bit more resilient and errors easier to decode, as well as pave the way to closing lots of other open issues (#41, #46, #47, #49, and #50).

This would break lots of things and we'd probably need to write a migration script for those on a <2.0.0 config version.

kevinschaich commented 4 years ago

@bennett39 tagging you here as well in case you have thoughts

bennett39 commented 4 years ago

@kevinschaich This looks reasonable, and obv you know the code/requirements better than I would. Agreed that if we're serious about moving to TypeScript, we ought to do it sooner rather than later.

Overall, while I see the value in TypeScript, I'm not sure how much benefit such a dramatic, breaking shift would actually get us. The open issues you linked would still require some work, even if we were magically converted to TypeScript.

Does migrating get us anything for free? Will it save us from eventual and forseeable headache down the road, or is it just "nice to have" it? (Legit asking, not rhetorical questions.)

kevinschaich commented 4 years ago

Hey @bennett39 apologies for the delay here but I think I'm ready to answer this now.

Typed code in general makes it easier to reason about where things will go wrong (hypothetically) and makes you a bit more rigorous in the definition/design phase. In Mintable 1.x.x, we give a "generic" definition of a provider, but to add a new one, you'd need to dig around through existing code and figure out what is going on (for Plaid, for example).

In the proposed API above this would be a lot easier – there is a clear expectation of what a provider will receive and give back to Mintable.

Also makes it easier to reason about configuration, for example in this commit, we can use the types above to generate and enforce a configuration schema on-the-fly:

$ export MINTABLE='{"integrations": [], "accounts": [], "balances": []}'

$ yarn mintable --config-variable=MINTABLE

2020-03-03T20:26:57.006Z [INFO] Using configuration variable `MINTABLE.`
2020-03-03T20:26:57.008Z [INFO] Successfully retrieved configuration variable.
2020-03-03T20:26:57.008Z [INFO] Successfully parsed configuration.
2020-03-03T20:26:59.194Z [ERROR] Unable to validate configuration.

 [
  {
    "keyword": "required",
    "dataPath": "",
    "schemaPath": "#/required",
    "params": {
      "missingProperty": "transactions"
    },
    "message": "should have required property 'transactions'"
  }
]

error Command failed with exit code 1.

More to come. Check out the release/2.0.0 branch to stay in the loop.

kevinschaich commented 4 years ago

Fixed in #66 – discussion in #55