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

Convert to TypeScript #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Convert to TypeScript #7

wants to merge 5 commits into from

Conversation

PaulBeaudet
Copy link
Member

This is a refactor to convert the onsite auth server from JavaScript to TypeScript.

// interface.ts Copyright 2020 Manchester Makerspace MIT License
import { Db, MongoClient } from 'mongodb';

interface cardDataI {
Copy link
Contributor

@lynch16 lynch16 Dec 15, 2020

Choose a reason for hiding this comment

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

Maybe take a look at the TS client I made. You don't really need the API calls I know, buit it has types for all our models in there https://github.com/ManchesterMakerspace/makerspace-ts-api-client

Using tree shaking, it shouldn't impact your prod build either. Benefit is that you can set things like "validity" to an enum

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good idea to share the types, I'll have to look into that a bit more tomorrow

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a number of not TypeScript first approaches to things in the auth server. For instance, the type for a rejected card and a valid one are partially conflated with each other for the sake of parameter passing convenience. Whereat the end of the checking chain for a valid card, if none is found the default card prop passed in {standing} is {holder: null, validity: "unregistered", expiry: null, uid: "string" } is passed as the rejection. Originally I kept holder and Expiry as falsy values of their perspective types, but that didn't work in prod in conjunction with the interface. Was one of the hotfixes during deployment if I remember correctly.

Do you think it would be appropriate that I add a CardBase for Card to extend from? The "id" prop isn't relevant until insertion and mongo would make it if it were missing regardless. It looks like "id" is consumed differently client-side than it's inserted as "_id", not sure why that is, but I'm sure TypeScript would complain on my side if I go out of my way to enforce the exact scheme of the insertion of a card.

I didn't notice types or properties for rejections in the API client module just the card and the validity enum. If wasn't needed for client-side cases that makes sense to me. I would want to add "unregistered" to the "CardValidity" enum and an a RejectedCard interface that extends from CardBase expecting the Null props because I don't expect the client-side would ever run into that and I could then just expect cardData to be RejectedCard | CardBase as a prop on the type Standing obj that I'm passing around in authorize().

I'm just thinking of ways to keep it dry. Maybe you have a better idea about how to approach it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea making a base type and extending it is a well respected pattern. If you don't want to define the null ones, its probably the best solution. If you do want to define the null fields, you could use a generic such as

interface CardData<T = string> {
  holder: T extends null ? null : string;
  validity: string;
  expiry: T extends null ? null : number;
  uid: string;
}

interface RejectionCard extends CardData<null> {}

const foobar: CardData = {
  holder: "foo",
  validity: "bar",
  expiry: 12,
  uid: "fizz"
};

const fizzbuzz: RejectionCard = {
  validity: "bar",
  uid: "fizz",
  expiry: null,
  holder: null,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there should be a Card type for you in the shared lib. You could make a rejection card easily with Pick<Card, "validity" | "uid">

export interface Card {
  id: string;
  holder: string;
  expiry: number;
  validity: string;
  uid: string;
}

Copy link
Member Author

@PaulBeaudet PaulBeaudet Dec 18, 2020

Choose a reason for hiding this comment

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

Card is what I was thinking of being an extension to CardBase. Based on the suggestion of using a generic the changes I would make to the TS API would look like this.

export enum CardValidity {
  ActiveMember = "activeMember",
  Expired = "expired",
  Inactive = "inactive",
  Lost = "lost",
  NonMember = "nonMember",
  Revoked = "revoked",
  Stolen = "stolen",
  Unregistered: "unregistered"
}

interface Card<T = string> {  // Mongo Doc
  holder: T extends null ? null | string : string;
  validity: string;
  expiry: T extends null ? null | number : number;
  uid: string;
  id: string;
}

type CacheCard = Omit<Card, "id">; // Cache Doc

interface RejectionCard extends Card<null> {
   timeOf: number;
} // Mongo Doc

type PotentialRejection = Omit<RejectionCard, "id" | "timeOf"> // Auth server card expectations when authenticating

Doorboto immediately destructures "Card" excluding "id/_id" when it populates the cache. After that CacheCard or PotentialRejection are the only types the auth server expects to see in terms of user credentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to just do this locally. The TS API is constructed from a swagger spec and won't work with generics.

You could do

import { Card as BaseCard } from "makerspace-ts-api-client";

interface Card<T = string> extends Omit<BaseCard, "holder" | "expiry"> {
  holder: T extends null ? null | string : string;
  expiry: T extends null ? null | number : number;
}

I think your strategy is sound though.

@PaulBeaudet
Copy link
Member Author

@lynch16 The only dependency I had a hard time getting the types for was @serialport/readline. So I ended up just using CommonJs and requiring it, it's likely just being read as 'any' as it stands. There is an open issue here DefinitelyTyped/DefinitelyTyped#30304. Seems like a contribution to serialport is being suggested to add the types file.

I might be missing something obvious though. What do you normally do when you can't find types for a dependency?

@lynch16
Copy link
Contributor

lynch16 commented Dec 16, 2020

@PaulBeaudet if there are no types for a module, you'll need to declare them. Here's an example of declaring some base Node modules

`declare module "url" {
export interface Url {
protocol?: string;
hostname?: string;
pathname?: string;
}

export function parse(
urlStr: string,
parseQueryString?,
slashesDenoteHost?
): Url;
}

declare module "path" {
export function normalize(p: string): string;
export function join(...paths: any[]): string;
export var sep: string;
}`

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.

2 participants