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

Migrate to TypeScript #27

Closed
kevinschaich opened this issue Apr 16, 2019 · 5 comments
Closed

Migrate to TypeScript #27

kevinschaich opened this issue Apr 16, 2019 · 5 comments
Assignees

Comments

@kevinschaich
Copy link
Owner

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

@kevinschaich kevinschaich added this to the Mintable 2.0 milestone Jan 30, 2020
@kevinschaich
Copy link
Owner Author

kevinschaich commented Jan 31, 2020

@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
Copy link
Owner Author

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

@bennett39
Copy link

@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
Copy link
Owner Author

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
Copy link
Owner Author

Fixed in #66 – discussion in #55

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants