Merge pull request #42 from grammarly/f-deprecate-propexpr

Deprecate property expressions

Author: blacktaxi

README.md

@@ -358,16 +358,22 @@ console.log(oneC.set(7, bigobj))
 Focal also gives you the ability to define these lenses quite conveniently:
 
 ```typescript
-// create the above lenses by ONLY providing a getter function¹
-const abc = Lens.prop((obj: typeof bigobj.one) => obj.a.b.c)
-
-const one = Lens.prop((obj: typeof bigobj) => obj.one)
-
-const two = Lens.prop((obj: typeof bigobj) => obj.two)
-
-// ¹ RESTRICTIONS APPLY! the getter function in this case can only be a simple
-// property path access function which consists of a single property access expression
-// and has no side effects.
+// create the above lenses by providing key names
+const abc = Lens.key<typeof bigobj.one>()('a', 'b', 'c')
+
+const one = Lens.key<typeof bigobj>()('one')
+
+const two = Lens.key<typeof bigobj>()('two')
+
+// Note the extra nullary call. It is there for better type inference. To create a type safe
+// lens we need to tell the compiler both the type of source and the type of destination property.
+// Unlike with Atom.lens, the compiler doesn't yet know the type of the source data, so we have to
+// explicitly state it. The compiler then can easily infer the type of the destination property by
+// its name.
+//
+// We need the nullary call because we have two generic type parameters, and want one of them
+// explicit and another inferred. So the nullary call is there only to supply the type argument
+// for the source data type.
 ```
 
 The best part about this is that it is completely type safe, and all of the IDE tools (like auto-completion, refactoring, etc.) will just work.
@@ -405,8 +411,8 @@ const obj = Atom.create({
   a: 5
 })
 
-// create a lens to look at an `a` property of the object
-const a = Lens.prop((x: typeof obj) => x.a)
+// create a lens to look at the `a` property of the object
+const a = Lens.key<typeof obj>()('a')
 
 // create a lensed atom that will hold a value of the `a` property of our object
 const lensed = obj.lens(a)
@@ -427,11 +433,7 @@ console.log(obj.get())
 Note how the source atom's value has changed when we set a new value to the lensed atom – that's it. There's also a neat shortcut to create lensed atoms:
 
 ```typescript
-const lensed = obj.lens(x => x.a) // ¹
-
-// ¹ SAME RESTRICTIONS APPLY! just like with Lens.prop, the getter function argument
-// to the atom's `lens` method can only be a simple property path access function which
-// consists of a single property access expression and has no side effects.
+const lensed = obj.lens('a')
 ```
 
 We don't need to explicitly create a `Lens` – atom's `lens` method already has a couple of overloads to create lensed atoms on the spot. Also note that we didn't need to add the `typeof obj` type annotation here: the compiler already knows the type of data we're operating on (from the type of `obj`, which in this case is `Atom<{ a: number }`) and can infer the type of `x` for us.
@@ -468,7 +470,7 @@ const App = (props: { state: Atom<{ count: number }> }) =>
       this is where we take apart the app state and give only a part of it
       to the counter component:
     */}
-    <Counter count={props.state.lens(x => x.count)} />
+    <Counter count={props.state.lens('count')} />
   </div>
 ```
 

packages/examples/all/src/player/index.tsx

@@ -65,11 +65,11 @@ class App extends React.Component<{ state: Atom<AppState> }, {}> {
     return (
       <div>
         <TimeLine
-          currentTime={state.lens(x => x.currentTime)}
-          maxDuration={state.view(x => x.maxDuration)}
+          currentTime={state.lens('currentTime')}
+          maxDuration={state.view('maxDuration')}
         />
-        <Volume volume={state.lens(x => x.volume)} />
-        <Play status={state.lens(x => x.status)} />
+        <Volume volume={state.lens('volume')} />
+        <Play status={state.lens('status')} />
       </div>
     )
   }

packages/examples/all/src/player/model.ts

@@ -36,7 +36,7 @@ export class AudioModel {
     this.durationSubscription = Observable
       .fromEvent(this.audio, 'canplaythrough')
       .subscribe(() =>
-        atom.lens(x => x.maxDuration).set(this.audio.duration)
+        atom.lens('maxDuration').set(this.audio.duration)
       )
 
     this.timeSubscription = atom
@@ -71,11 +71,11 @@ export class AudioModel {
       )
 
     this.volumeSubscription = atom
-      .lens(x => x.volume)
+      .lens('volume')
       .subscribe(x => this.audio.volume = x / 10)
 
     this.currentTimeSubscription = atom
-      .lens(x => x.currentTime)
+      .lens('currentTime')
       .subscribe(x => this.audio.currentTime = parseInt(x, 10))
   }
 

packages/focal/src/atom/base.ts

@@ -164,6 +164,8 @@ export interface Atom<T> extends ReadOnlyAtom<T> {
   set(newValue: T): void
 
   /**
+   * DEPRECATED: please use other overloads instead!
+   *
    * Create a lensed atom using a property expression, which specifies
    * a path inside the atom value's data structure.
    *

packages/focal/src/lens/json.ts

@@ -9,7 +9,9 @@ import {
   setKey,
   conservatively,
   findIndex,
-  Option
+  Option,
+  DEV_ENV,
+  warning
 } from './../utils'
 
 import { Lens, Prism } from './base'
@@ -151,13 +153,35 @@ export function keyImpl<TObject>(k?: string) {
     )
 }
 
+let propExprDeprecatedWarnings = 0
+
+function warnPropExprDeprecated(path: string[]) {
+  // don't warn more than a few times
+  if (propExprDeprecatedWarnings < 10) {
+    propExprDeprecatedWarnings++
+
+    const propExpr = `x.${path.join('.')}`
+    const keys = `'${path.join("', '")}'`
+
+    warning(
+      `The property expression overload of Atom.lens and Lens.prop are deprecated and ` +
+      `will be removed in next versions of Focal. Please use the key name overload for ` +
+      `Atom.lens and Lens.key instead. ` +
+      `You can convert your code by changing the calls:
+  a.lens(x => ${propExpr}) to a.lens(${keys}),
+  Lens.prop((x: T) => ${propExpr}) to Lens.key<T>()(${keys}).`
+    )
+  }
+}
+
 export function propImpl<TObject, TProperty>(
   getter: PropExpr<TObject, TProperty>
 ): Lens<TObject, TProperty> {
+  const path = extractPropertyPath(getter as PropExpr<TObject, TProperty>)
+  if (DEV_ENV) warnPropExprDeprecated(path)
+
   // @TODO can we optimize this?
-  return Lens.compose<TObject, TProperty>(
-    ...extractPropertyPath(getter as PropExpr<TObject, TProperty>)
-      .map(keyImpl()))
+  return Lens.compose<TObject, TProperty>(...path.map(keyImpl()))
 }
 
 export function indexImpl<TItem>(i: number): Prism<TItem[], TItem> {
@@ -216,6 +240,8 @@ declare module './base' {
     export let key: typeof keyImpl
 
     /**
+     * DEPRECATED: please use Lens.key instead!
+     *
      * Create a lens to an object's property. The argument is a property expression, which
      * is a limited form of a getter, with following restrictions:
      * - should be a pure function

packages/focal/src/react/intrinsic.ts

@@ -33,7 +33,7 @@ export type GenericLiftedIntrinsic<T> =
     : never
 
 export type LiftedIntrinsicsHTML = {
-  [K in keyof React.ReactHTML]: GenericLiftedIntrinsic<React.ReactHTML[K]>
+  readonly [K in keyof React.ReactHTML]: GenericLiftedIntrinsic<React.ReactHTML[K]>
 }
 
 export interface LiftedFragmentAttributes extends ObservableReactChildren, React.Attributes {}
@@ -44,13 +44,11 @@ export interface LiftedFragment {
     React.ReactElement<LiftWrapperProps<ObservableReactHTMLProps<{}>>>
 }
 
-export const enum ExtendedIntrinsics {
-  fragment = 'Fragment'
+interface ExtraLiftedIntrinsics {
+  readonly Fragment: LiftedFragment
 }
 
-export type LiftedIntrinsics = LiftedIntrinsicsHTML & {
-  [k in ExtendedIntrinsics.fragment]: LiftedFragment
-}
+export type LiftedIntrinsics = LiftedIntrinsicsHTML & ExtraLiftedIntrinsics
 
 export function createLiftedIntrinsics(): LiftedIntrinsics {
   const html: (keyof LiftedIntrinsicsHTML)[] = [
@@ -67,14 +65,14 @@ export function createLiftedIntrinsics(): LiftedIntrinsics {
     'var', 'video', 'wbr'
   ]
 
-  const r = {} as any
-  html.forEach(e => r[e] = liftIntrinsic(e))
+  const r: {
+    -readonly [P in keyof LiftedIntrinsics]?: LiftedIntrinsics[P];
+  } = {}
 
-  const fragmentTag: LiftedIntrinsics[ExtendedIntrinsics.fragment] =
-    (props: LiftedFragmentAttributes) =>
-      React.createElement(LiftWrapper, { component: React.Fragment, props })
+  html.forEach(e => r[e] = liftIntrinsic(e))
 
-  r[ExtendedIntrinsics.fragment] = fragmentTag
+  r.Fragment = (props: LiftedFragmentAttributes) =>
+    React.createElement(LiftWrapper, { component: React.Fragment, props })
 
   return r as LiftedIntrinsics
 }