Docdiff: investigate why it breaks other integrations

[humitos]

Docdiff works correctly currently. However, I had to disable it for now because it interferes with other integrations like EthicalAds and Search As You Type.

I assume that it breaks because it changes the DOM and it may be removing some things required for the other integrations? 🤷🏼

[agjohnson]

This is probably because doc-diff replaces the DOM at config.root_selector:

https://github.com/readthedocs/doc-diff/blob/main/src/extension.js#L166

It should probably be loaded last, after all other DOM manipulations are finalized.

[humitos]

It should probably be loaded last, after all other DOM manipulations are finalized.

That's why I've tried by putting it as the last item in the promises array, but it seems that's not enough since all the promises in the array are run concurrently.

I was trying to chain a final .then() for the docdiff in particular, but I'm not finding a clean way to pass the config object:

...
        return Promise.all(promises);
      })
      .then(() => {
        initializeDocDiff(config);
      })
      .then(() => {
        resolve();
      })
....

There config is not defined because it's the first attribute of the second .then() and it's not returning the config, but the Promise.all() call. Am I missing something here? What would be a good pattern for this?

[humitos]

Hrm... I just realized that all those promises are working in a sync way because I'm calling them directly. I have to create a new Promise() if I want them to be async.

[agjohnson]

Well, you have three layers of promises so far: the outside promise signalling loading state, the promises for each tool, and the return from Promise.all(...) (this is also a promise ¹). You'd want to chain on to the return value from Promise.all(...), where you do have config scoped. So, something like:

return Promise.all(
  promises
).then(() => {
  return docDiff.initialize(config);
});

Footnotes

1. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all#return_value ↩

[agjohnson]

I have to create a new Promise() if I want them to be async.

I didn't look closely, but this is generally the issue that makes async code hard. Everything needs to be async to take advantage of async operations. For the purposes of this first iteration, it will be easier to avoid promises/async execution.

[humitos]

I updated the code to make it a little cleaner and show what I'm trying to do and how I'm chaining the Promises. It still doesn't work, because for some reason is not calling initializeDocDiff now, but 🤷🏼

https://github.com/readthedocs/readthedocs-client/blob/a78b2fd74b5b783cb6b5c326a70f0fe438f8169d/src/index.js#L73-L77

[agjohnson]

It looks like you're not resolving the promises here: https://github.com/readthedocs/readthedocs-client/blob/main/src/index.js#L67-L69

This would leave the Promise waiting for resolve/reject, and your then(...) will never be reached. You will need to call that resolve callback function to tell the Promise to resolve. Something like:

for (let fn of integrations) {
  promises.push(
    new Promise((resolve) => {
      resolve(fn(config));
    })
  );
}

Promise also gets a reject argument on the promise callback. This can be used to trigger error handling farther up on the Promise chain:

return new Promise((resolve, reject) => {
   ...
   reject(new Error("Oops!"));
});

Promises tend to have a steep learning curve. Native async code is a bit easier to understand in comparison, as there are lots of ways to shoot your foot with promises.