Skip to content

Instantly share code, notes, and snippets.

@OliverJAsh

OliverJAsh/foo.md

Last active Aug 20, 2019
Embed
What would you like to do?
Dev UX: auto imports Dev UX: contextual naming Perf: tree shaking Dev UX: namespace/type merging
namespace import not yet yes maybe no
TS namespace yes yes no yes
types: namespace import / values: named import not yet / yes yes / no yes no
types: TS namespace / values: named import yes yes / no yes yes
@karol-majewski

This comment has been minimized.

Copy link

@karol-majewski karol-majewski commented Aug 19, 2019

What we want:

  • tree shaking
  • sweet syntax (type-namespace considered nice, fewer imports — like 1 — considered nice)
  • namespace type-merging?
  • no contextual naming? (context provided only once, don't be able to import checkIsFull without any context)
  • no Hungarian notation (no repeating Type or Entity inside the type name)
  • no naming conflicts (what do we rename?)
  • VSCode support (auto-imports, find references, rename symbol — asterisk imports/default exports)
  • Works with Unionized unions
@OliverJAsh

This comment has been minimized.

Copy link
Owner Author

@OliverJAsh OliverJAsh commented Aug 19, 2019

@karol-majewski

This comment has been minimized.

Copy link

@karol-majewski karol-majewski commented Aug 20, 2019

Final thoughts

  • Use the namespace keyword to aggregate types
  • Separate the API definition from application-specific types
  • Don't use namespace imports

Table of contents

Code sample

unsplash-web/src/services/Unsplash.ts
import { Unsplash } from "unsplash";
import { Collection, User, Photo } from "../entities";

/**
 * Type guards.
 */
function isCollection(candidate: any): Unsplash.Collection;
function isUser(candidate: any): Unsplash.User;
function isPhoto(candidate: any): Unsplash.Photo;

/**
 * Adapters
 */
function normalize(entity: Unsplash.Collection): Collection;
function normalize(entity: Unsplash.User): User;
function normalize(entity: Unsplash.Photo): Photo {}

/**
 * An HTTP client using the Unsplash API definition.
 */
enum Endpoints {
  Users = "/users/"
  User = "/user/:id"
}

export class Client {
  constructor(private baseUrl: string) {}

  get(endpoint: Endpoint.Users): Promise<User[]>;
  get(endpoint: Endpoint.User, id: string): Promise<User> {
    return fetch(endpoint).then(normalize);
  }
}
unsplash-web/src/side-effects.ts
import { Client as UnsplashClient } from "../services/Unsplash";

const unsplash = new UnsplashClient(process.env.UNSPLASH_API_URL);

unsplash
  .get("user", "q1w2e3r4")
  .then(validate)
  .then(synchronizeWithReduxStore);

Dos and don'ts

DO use the namespace keyword to aggregate types

// Bad
interface CollectionBasic {}
interface CollectionExtended {}

declare const myCollection: CollectionExtended;
// Good
namespace Collection {
  interface Basic {}
  interface Extended {}
}

declare const myCollection: Collection.Extended;
  • Easier to read
  • Hierarchies are clear

DO NOT use the namespace keyword to aggregate values

// Bad
namespace Collection {
  interface Basic {}
  interface Extended {}

  function isBasic(candidate: Collection): candidate is Collection.Basic {}
}
// Good
namespace Collection {
  interface Basic {}
  interface Extended {}
}

export function isBasic(collection: Collection): candidate is Collection.Basic {}
  • When a namespace contains values, the generated code includes bloated, non-tree-shakeable
    objects
  • Overloading identifiers in such a way is non-idiomatic and
    non-affordable. When you see a Collection, what can
    you do with it? Is it an interface? Is it a namespace? Is it a class? Should you call it,
    instantiate it, access its members?
  • Enforce with TSLint:"no-namespace": [true, "allow-declarations"]

DO separate your API schema from application-specific types

  • The API schema is unlikely to change. Application-specific entities are allowed to change. That's
    where you should put a boundary.
  • Conceptually, the relationship between the API and the client is one-directional. The client
    depends on the API. The API is agnostic towards its clients. Separating your API types from
    application-specific types reflects that relationship.
  • Easy to extract from this repository (to put it on npm, version, and share with others)
  • The API team can maintain it, deploy it, and guarantee its correctness
  • We don't know what the possible-feature-maybe-one-day SDK should consist of. We don't need to make
    that decision now. We can keep our helpers where they are right now — in our repository.

DO use declaration (d.ts) files to store your types

  • TypeScript doesn't allow executable code to live in declaration files
  • No risk of breaking tree-shaking by accidentally importing types and values via namespace import

DO use declaration merging to organize sibling types

// Bad
declare namespace Collection {
  interface Basic {}
  interface Extended {}
}

function isBasic(collection: Collection.Basic | Collection.Extended): collection is Collection.Basic;
// Better
declare namespace Collection {
  interface Basic {}
  interface Extended {}

  type Any = Basic | Extended;
}

function isBasic(collection: Collection.Any): collection is Collection.Basic;
// Best
declare namespace Collection {
  interface Basic {}
  interface Extended {}
}

type Collection = Collection.Basic | Collection.Extended;

function isBasic(collection: Collection): collection is Collection.Basic;
  • Easy to read
  • Doesn't need clumsy suffixes for disambiguation (like Collection.Type or Collection.Union)
  • Highlights the relationship (sibling variations of one type)

DO NOT pick a "default" entity when aggregating types

// Bad
declare namespace Collection {
  interface Basic {}
  interface Extended {}
}

type Collection = Collection.Basic;
// Good
declare namespace Collection {
  interface Basic {}
  interface Extended {}
}

type Collection = Collection.Basic | Collection.Extended;
  • It's more predictable if we do it the same way everywhere
  • Future proof — after all, you might change your mind as to what the "default" entity should be.

DO NOT use namespace imports

// Bad
import Collection from "some-library";
import * as Collections from "unsplash/lib/collections";

const collections: Collection<Collections.Collection> = [];
// Better
import Collection from "some-library";
import { Collection as UnsplashCollection } from "unsplash/lib/collections";

const collections: Collection<UnsplashCollection> = [];
// Best
import Collection from "some-library";
import { Unsplash } from "unsplash";

const collections: Collection<Unsplash.Collection> = [];
  • Namespace imports lead to inconsistent naming (because you can pick whatever name you want)
  • Named exports are more robust when it comes to tree shaking. Namespace imports create the risk of
    accidentally import types and values (see above)
    No longer the case?
  • Named exports always work with IDE features like renaming, importing and jumping to the definition.
  • Namespace imports lead to clumsy names like Collection.Collection and there is no way to reduce
    the amount of context. We should add no more context to a name than is necessary. To the contrary,
    named exports can be given more context if they don't have enough
    (import { Collection as UnsplashCollection } from 'unsplash').

Checklist

  • Third-party entities (here: Unsplash API entities) are pushed to the boundary of the application.
    • They are validated, normalized and purified there.
    • They don't spread.
  • All the requirements are met:
    • Tree shaking is guaranteed to work
    • The syntax is sweet
    • Contextual naming: there's never too much context. The amount of context can be increased
      when needed.
    • No clumsy suffixes like Type or Entity in the type name
    • No avoidable naming conflicts
    • Developer experience (auto-imports, find references, jump to definition, rename symbol)
    • Works with Unionize unions (?) (define works)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment