[WIP] Stop waiting for sourcemap sources

[juliandescottes]

Not ready for review, just want to trigger CI and see if I broke the world with this.

Attempts to fix #4654 and #4705

[codehag]

i mentioned it in the issue but i will mention it here as well: removing awaits can be a bit dangerous here, especially in regards to loadSourceText. maybe what we should do is create a queuing system for loadSourceText, so that we can call it without worry. ie) if the same file is being loaded twice, and we are in the process of loading it, wait for the promise with the given source id to load. might look something like this

const queue ={};

//...
export function loadSourceText(...) {
// ...
if (source.text) {
  return source.text;
}
const queuedRequest = queue[source.id];

if (queuedRequest) {
  return queuedRequest;
}

queue[source.id] = ( ... // promise)

in the promise resolution we should delete the queued item.

[juliandescottes]

thanks for the suggestion! I'll try to implement that. I will close the PR as I don't think we'll go with anything remotely similar to this. Interesting (and scary ? :) ) to see that no test failed.

[jasonLaster]

Opening so we can iterate here.

[jasonLaster]

i took a look and i think the concern is w/ load source text, which is easy to check for...

pending bps, might trigger more loads or simultaneous syncs, but that should be safe.

gathering some docs on profiling redux so you can see the sequence of actions

diff --git a/src/actions/sources.js b/src/actions/sources.js
index 79df46b..c9e2b5e 100644
--- a/src/actions/sources.js
+++ b/src/actions/sources.js
@@ -133,9 +133,9 @@ function loadSourceMap(generatedSource) {
 
     dispatch({ type: "ADD_SOURCES", sources: originalSources });
 
-    await dispatch(loadSourceText(generatedSource));
+    dispatch(loadSourceText(generatedSource));
     originalSources.forEach(async source => {
-      await checkSelectedSource(getState(), dispatch, source);
+      checkSelectedSource(getState(), dispatch, source);
       checkPendingBreakpoints(getState(), dispatch, source.id);
     });
   };
diff --git a/src/actions/sources/loadSourceText.js b/src/actions/sources/loadSourceText.js
index ad87ea2..81c63e3 100644
--- a/src/actions/sources/loadSourceText.js
+++ b/src/actions/sources/loadSourceText.js
@@ -11,6 +11,8 @@ import { setSource } from "../../workers/parser";
 import type { Source } from "../../types";
 import type { ThunkArgs } from "../types";
 
+import { isLoaded, isLoading } from "../../utils/source";
+
 async function loadSource(source: Source, { sourceMaps, client }) {
   if (sourceMaps.isOriginalId(source.id)) {
     return await sourceMaps.getOriginalSourceText(source);
@@ -32,7 +34,7 @@ async function loadSource(source: Source, { sourceMaps, client }) {
 export function loadSourceText(source: Source) {
   return async ({ dispatch, getState, client, sourceMaps }: ThunkArgs) => {
     // Fetch the source text only once.
-    if (source.text) {
+    if (isLoading(source) || isLoaded(source)) {
       return Promise.resolve(source);
     }
 
diff --git a/src/utils/source.js b/src/utils/source.js
index f1e2e37..8439da1 100644
--- a/src/utils/source.js
+++ b/src/utils/source.js
@@ -263,6 +263,10 @@ function isLoaded(source: Source) {
   return source.loadedState === "loaded";
 }
 
+function isLoading(source: Source) {
+  return source.loadedState === "loading";
+}
+
 export {
   isJavaScript,
   isPretty,
@@ -276,5 +280,6 @@ export {
   getSourcePath,
   getSourceLineCount,
   getMode,
-  isLoaded
+  isLoaded,
+  isLoading
 };

[jasonLaster]

I pushed a small version w/ queuing, which is a good short term solution.

The benefit of this approach is that it makes the simple case of loading a source map as fast as possible.

In the situation where there is a breakpoint in an original file, we will need both sources (original, generated) to be loaded. We can add these checks where we need them in checkPendingBreakpoints we didn't do this before because we didn't have queuing, but now it is reasonable.

We probably could go a couple step further and remove similar await checks. We should probably only try this if the first part makes sense.

diff --git a/src/actions/sources.js b/src/actions/sources.js
index af77b3d..fc7f319 100644
--- a/src/actions/sources.js
+++ b/src/actions/sources.js
     dispatch({ type: "ADD_SOURCE", source });

     if (prefs.clientSourceMapsEnabled) {
-      await dispatch(loadSourceMap(source));
+      dispatch(loadSourceMap(source));
     }

-    await checkSelectedSource(getState(), dispatch, source);
-    await checkPendingBreakpoints(getState(), dispatch, source.id);
+    checkSelectedSource(getState(), dispatch, source);
+    checkPendingBreakpoints(getState(), dispatch, source.id);
   };
 }

@@ -102,7 +103,7 @@ export function newSources(sources: Source[]) {
     );

     for (const source of filteredSources) {
-      await dispatch(newSource(source));
+      dispatch(newSource(source));
     }
   };
 }

A good next step would be to make sure the mochitests and jest tests still pass. When they do, we can make sure we have the appropriate coverage. Fortunately, this is mostly coverage for pending breakpoints which we do test quite a bit already in both jest src/actions/tests/pending-breakpoints.spec and in mochitests with reloading for source-maps-reloading

[jasonLaster]

left a writeup in the original issue

[jasonLaster]

This PR builds on #4753 #4749

• Throttle new sources
• only load sources once

And adds some extra super powers for source maps.

• previously, each new source would have it's source map fetched and original sources add synchronously. This creates potentially hundreds of extra store updates...
• now, all of the new sources original sources are fetched and loaded in-batch. This reduces the number of updates to the store.
• newSources is moved to it's own module and the order of operations are optimized so that the user sees most of the updates as quickly as possible.

The intuition for this algorithm came from Chrome Canary's timeline, which shows significant gaps between addSources calls... while in theory updating the store should be cheap, in practice it is something we definitely want to minimize. Also, putting all the rules in newSources feels good because we continue to make this algorithm more sophisticated.

[jasonLaster]

I updated this PR to include some pretty significant refactoring for the sources actions:

the actions got the pause actions treatment so each type of action selectSource, newSource, ... got its own module. This makes it easier to see how each use case is suppose to work and what modules they import. It is also easier to test and see what the test/flow coverage is

• togglePrettyPrint had to be in selectSource because of circular dependencies
• addTab was removed because of circular deps. I feel like tabs should be its own reducer, but not necessarily have actions... the reducer should just listen to source actions
• the tests now listen for some of the async behavior, for instance pending breakpoints needs to wait for a breakpoint to be registered because you cant assume it has been registered when newSource is done. There was also a module mock (loadSourceText), which i removed because it simplifed the test and we don't use module mocks for action tests in this part of the suite. The action tests as they exist now are designed to be more similar to integration tests, but i would be happy to setup some more specific unit tests w/ better mocking to isolate some of the new hairy logic. especially as we boost the coverage.
• the refactor is bringing out interesting action couplings, which ofcourse makes me think of sagas. Lets talk. Hopefully this gets us closer to sagas. And now that we're at the beginning of a cycle (59) we can see if sagas can help address some of the difficult logic here.

Next Steps:

• unit tests pass, but i'm sure flow and mochitests will need some love.
• everything is separated, but it'd be good to see if it makes sense as is. I could see some things moving around.
• every module imports everything, sorry :) will need some cleanup there

[codehag]

some parts of this have been broken out already, and it is too large of a change to merge in. What else can we break out so that this pr can be closed?

[jasonLaster]

This is a tough one because I made it into a refactor actions/sources too :)

I think the first 3 commits can been broken out into separate PRs. The last two have some changes for functionality, but it's very minimal.

[jasonLaster]

closing this as all of the work has been moved to separate PRs except for the refactor, which i'll re-open.