Bug 1541631 - Part 7: Convert memoizeableAction to be AsyncValue-based for easy interop. r=jlast
☠☠ backed out by db9aaec44445 ☠ ☠
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 15 Aug 2019 16:29:15 +0000
changeset 488271 696e24030686ebe58588d5415b932077a6af0c11
parent 488270 6db731f26958d5f6ab2f6b3166a3d77e4cfeacaa
child 488272 bc99fb7a9d125b00d33e54d002facbef7e8e3f2b
push id36440
push userncsoregi@mozilla.com
push dateFri, 16 Aug 2019 03:57:48 +0000
treeherdermozilla-central@a58b7dc85887 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1541631
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1541631 - Part 7: Convert memoizeableAction to be AsyncValue-based for easy interop. r=jlast Differential Revision: https://phabricator.services.mozilla.com/D42032
devtools/client/debugger/src/actions/breakpoints/breakpointPositions.js
devtools/client/debugger/src/actions/sources/loadSourceText.js
devtools/client/debugger/src/actions/sources/symbols.js
devtools/client/debugger/src/reducers/sources.js
devtools/client/debugger/src/utils/async-value.js
devtools/client/debugger/src/utils/memoizableAction.js
--- a/devtools/client/debugger/src/actions/breakpoints/breakpointPositions.js
+++ b/devtools/client/debugger/src/actions/breakpoints/breakpointPositions.js
@@ -9,18 +9,16 @@ import {
   isGeneratedId,
   originalToGeneratedId,
 } from "devtools-source-map";
 import { uniqBy, zip } from "lodash";
 
 import {
   getSource,
   getSourceFromId,
-  hasBreakpointPositions,
-  hasBreakpointPositionsForLine,
   getBreakpointPositionsForSource,
   getSourceActorsForSource,
 } from "../../selectors";
 
 import type {
   MappedLocation,
   Range,
   SourceLocation,
@@ -28,16 +26,17 @@ import type {
   Context,
 } from "../../types";
 
 import { makeBreakpointId } from "../../utils/breakpoint";
 import {
   memoizeableAction,
   type MemoizedAction,
 } from "../../utils/memoizableAction";
+import { fulfilled } from "../../utils/async-value";
 import type { ThunkArgs } from "../../actions/types";
 
 async function mapLocations(
   generatedLocations: SourceLocation[],
   { sourceMaps }: ThunkArgs
 ) {
   if (generatedLocations.length == 0) {
     return [];
@@ -192,21 +191,29 @@ function generatedSourceActorKey(state, 
     : [];
   return [sourceId, ...actors].join(":");
 }
 
 export const setBreakpointPositions: MemoizedAction<
   { cx: Context, sourceId: string, line?: number },
   ?BreakpointPositions
 > = memoizeableAction("setBreakpointPositions", {
-  hasValue: ({ sourceId, line }, { getState }) =>
-    isGeneratedId(sourceId) && line
-      ? hasBreakpointPositionsForLine(getState(), sourceId, line)
-      : hasBreakpointPositions(getState(), sourceId),
-  getValue: ({ sourceId, line }, { getState }) =>
-    getBreakpointPositionsForSource(getState(), sourceId),
+  getValue: ({ sourceId, line }, { getState }) => {
+    const positions = getBreakpointPositionsForSource(getState(), sourceId);
+    if (!positions) {
+      return null;
+    }
+
+    if (isGeneratedId(sourceId) && line && !positions[line]) {
+      // We always return the full position dataset, but if a given line is
+      // not available, we treat the whole set as loading.
+      return null;
+    }
+
+    return fulfilled(positions);
+  },
   createKey({ sourceId, line }, { getState }) {
     const key = generatedSourceActorKey(getState(), sourceId);
     return isGeneratedId(sourceId) && line ? `${key}-${line}` : key;
   },
   action: async ({ cx, sourceId, line }, thunkArgs) =>
     _setBreakpointPositions(cx, sourceId, line, thunkArgs),
 });
--- a/devtools/client/debugger/src/actions/sources/loadSourceText.js
+++ b/devtools/client/debugger/src/actions/sources/loadSourceText.js
@@ -14,17 +14,17 @@ import {
   getSourcesEpoch,
   getBreakpointsForSource,
   getSourceActorsForSource,
 } from "../../selectors";
 import { addBreakpoint } from "../breakpoints";
 
 import { prettyPrintSource } from "./prettyPrint";
 import { setBreakableLines } from "./breakableLines";
-import { isFulfilled } from "../../utils/async-value";
+import { isFulfilled, fulfilled } from "../../utils/async-value";
 
 import { isOriginal, isPretty } from "../../utils/source";
 import {
   memoizeableAction,
   type MemoizedAction,
 } from "../../utils/memoizableAction";
 
 import { Telemetry } from "devtools-modules";
@@ -132,26 +132,31 @@ export function loadSourceById(cx: Conte
     return dispatch(loadSourceText({ cx, source }));
   };
 }
 
 export const loadSourceText: MemoizedAction<
   { cx: Context, source: Source },
   ?Source
 > = memoizeableAction("loadSourceText", {
-  hasValue: ({ source }, { getState }) => {
-    return (
-      !source ||
-      !!(
-        getSource(getState(), source.id) &&
-        getSourceWithContent(getState(), source.id).content
-      )
-    );
+  getValue: ({ source }, { getState }) => {
+    source = source ? getSource(getState(), source.id) : null;
+    if (!source) {
+      return null;
+    }
+
+    const { content } = getSourceWithContent(getState(), source.id);
+    if (!content || content.state === "pending") {
+      return content;
+    }
+
+    // This currently swallows source-load-failure since we return fulfilled
+    // here when content.state === "rejected". In an ideal world we should
+    // propagate that error upward.
+    return fulfilled(source);
   },
-  getValue: ({ source }, { getState }) =>
-    source ? getSource(getState(), source.id) : null,
   createKey: ({ source }, { getState }) => {
     const epoch = getSourcesEpoch(getState());
     return `${epoch}:${source.id}`;
   },
   action: ({ cx, source }, thunkArgs) =>
     loadSourceTextPromise(cx, source, thunkArgs),
 });
--- a/devtools/client/debugger/src/actions/sources/symbols.js
+++ b/devtools/client/debugger/src/actions/sources/symbols.js
@@ -1,24 +1,25 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
 
-import { hasSymbols, getSymbols } from "../../selectors";
+import { getSymbols } from "../../selectors";
 
 import { PROMISE } from "../utils/middleware/promise";
 import { updateTab } from "../tabs";
 import { loadSourceText } from "./loadSourceText";
 
 import {
   memoizeableAction,
   type MemoizedAction,
 } from "../../utils/memoizableAction";
+import { fulfilled } from "../../utils/async-value";
 
 import type { Source, Context } from "../../types";
 import type { Symbols } from "../../reducers/types";
 
 async function doSetSymbols(cx, source, { dispatch, getState, parser }) {
   const sourceId = source.id;
 
   await dispatch(loadSourceText({ cx, source }));
@@ -36,16 +37,24 @@ async function doSetSymbols(cx, source, 
   }
 }
 
 type Args = { cx: Context, source: Source };
 
 export const setSymbols: MemoizedAction<Args, ?Symbols> = memoizeableAction(
   "setSymbols",
   {
-    hasValue: ({ source }, { getState }) =>
-      source.isWasm || hasSymbols(getState(), source),
-    getValue: ({ source }, { getState }) =>
-      source.isWasm ? null : getSymbols(getState(), source),
+    getValue: ({ source }, { getState }) => {
+      if (source.isWasm) {
+        return fulfilled(null);
+      }
+
+      const symbols = getSymbols(getState(), source);
+      if (!symbols || symbols.loading) {
+        return null;
+      }
+
+      return fulfilled(symbols);
+    },
     createKey: ({ source }) => source.id,
     action: ({ cx, source }, thunkArgs) => doSetSymbols(cx, source, thunkArgs),
   }
 );
--- a/devtools/client/debugger/src/reducers/sources.js
+++ b/devtools/client/debugger/src/reducers/sources.js
@@ -254,17 +254,17 @@ function update(
 
 const resourceAsSourceBase = memoizeResourceShallow(
   ({ content, ...source }: SourceResource): SourceBase => source
 );
 
 const resourceAsSourceWithContent = memoizeResourceShallow(
   ({ content, ...source }: SourceResource): SourceWithContent => ({
     ...source,
-    content: content && content.state !== "pending" ? content : null,
+    content: asyncValue.asSettled(content),
   })
 );
 
 /*
  * Add sources to the sources store
  * - Add the source to the sources store
  * - Add the source URL to the urls map
  */
@@ -783,22 +783,17 @@ export function getSourceWithContent(
     resourceAsSourceWithContent
   );
 }
 export function getSourceContent(
   state: OuterState,
   id: SourceId
 ): SettledValue<SourceContent> | null {
   const { content } = getResource(state.sources.sources, id);
-
-  if (!content || content.state === "pending") {
-    return null;
-  }
-
-  return content;
+  return asyncValue.asSettled(content);
 }
 
 export function getSelectedSourceId(state: OuterState) {
   const source = getSelectedSource((state: any));
   return source && source.id;
 }
 
 export function getProjectDirectoryRoot(state: OuterState): string {
--- a/devtools/client/debugger/src/utils/async-value.js
+++ b/devtools/client/debugger/src/utils/async-value.js
@@ -23,16 +23,22 @@ export function pending(): PendingValue 
 }
 export function fulfilled<+T>(value: T): FulfilledValue<T> {
   return { state: "fulfilled", value };
 }
 export function rejected(value: mixed): RejectedValue {
   return { state: "rejected", value };
 }
 
+export function asSettled<T>(
+  value: AsyncValue<T> | null
+): SettledValue<T> | null {
+  return value && value.state !== "pending" ? value : null;
+}
+
 export function isPending(value: AsyncValue<mixed>): boolean %checks {
   return value.state === "pending";
 }
 export function isFulfilled(value: AsyncValue<mixed>): boolean %checks {
   return value.state === "fulfilled";
 }
 export function isRejected(value: AsyncValue<mixed>): boolean %checks {
   return value.state === "rejected";
--- a/devtools/client/debugger/src/utils/memoizableAction.js
+++ b/devtools/client/debugger/src/utils/memoizableAction.js
@@ -1,29 +1,31 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
 
 import type { ThunkArgs } from "../actions/types";
+import { asSettled, type AsyncValue } from "./async-value";
 
-export type MemoizedAction<Args, Result> = Args => ThunkArgs => Promise<Result>;
+export type MemoizedAction<
+  Args,
+  Result
+> = Args => ThunkArgs => Promise<Result | null>;
 type MemoizableActionParams<Args, Result> = {
-  hasValue: (args: Args, thunkArgs: ThunkArgs) => boolean,
-  getValue: (args: Args, thunkArgs: ThunkArgs) => Result,
+  getValue: (args: Args, thunkArgs: ThunkArgs) => AsyncValue<Result> | null,
   createKey: (args: Args, thunkArgs: ThunkArgs) => string,
   action: (args: Args, thunkArgs: ThunkArgs) => Promise<mixed>,
 };
 
 /*
  * memoizableActon is a utility for actions that should only be performed
  * once per key. It is useful for loading sources, parsing symbols ...
  *
- * @hasValue - checks to see if the result is in the redux store
  * @getValue - gets the result from the redux store
  * @createKey - creates a key for the requests map
  * @action - kicks off the async work for the action
  *
  *
  * For Example
  *
  * export const setItem = memoizeableAction(
@@ -34,41 +36,52 @@ type MemoizableActionParams<Args, Result
  *     createKey: ({ a }) => a,
  *     action: ({ a }, thunkArgs) => doSetItem(a, thunkArgs)
  *   }
  * );
  *
  */
 export function memoizeableAction<Args, Result>(
   name: string,
-  {
-    hasValue,
-    getValue,
-    createKey,
-    action,
-  }: MemoizableActionParams<Args, Result>
+  { getValue, createKey, action }: MemoizableActionParams<Args, Result>
 ): MemoizedAction<Args, Result> {
   const requests = new Map();
-  return args => async (thunkArgs: ThunkArgs) => {
-    if (hasValue(args, thunkArgs)) {
-      return getValue(args, thunkArgs);
+  return args => async thunkArgs => {
+    let result = asSettled(getValue(args, thunkArgs));
+    if (!result) {
+      const key = createKey(args, thunkArgs);
+      if (!requests.has(key)) {
+        requests.set(
+          key,
+          (async () => {
+            try {
+              await action(args, thunkArgs);
+            } catch (e) {
+              console.warn(`Action ${name} had an exception:`, e);
+            } finally {
+              requests.delete(key);
+            }
+          })()
+        );
+      }
+
+      await requests.get(key);
+
+      result = asSettled(getValue(args, thunkArgs));
+
+      if (!result) {
+        // Returning null here is not ideal. This means that the action
+        // resolved but 'getValue' didn't return a loaded value, for instance
+        // if the data the action was meant to store was deleted. In a perfect
+        // world we'd throw a ContextError here or handle cancellation somehow.
+        // Throwing will also allow us to change the return type on the action
+        // to always return a promise for the getValue AsyncValue type, but
+        // for now we have to add an additional '| null' for this case.
+        return null;
+      }
     }
 
-    const key = createKey(args, thunkArgs);
-    if (!requests.has(key)) {
-      requests.set(
-        key,
-        (async () => {
-          try {
-            await action(args, thunkArgs);
-          } catch (e) {
-            console.warn(`Action ${name} had an exception:`, e);
-          } finally {
-            requests.delete(key);
-          }
-        })()
-      );
+    if (result.state === "rejected") {
+      throw result.value;
     }
-
-    await requests.get(key);
-    return getValue(args, thunkArgs);
+    return result.value;
   };
 }