LeaVerou / inspire.js

Lean, hackable, extensible slide deck framework. Previously known as CSSS.
https://inspirejs.org
MIT License
1.73k stars 254 forks source link

[Regression] Plugins are loaded too late: after the init process is done #161

Closed DmitrySharabin closed 2 months ago

DmitrySharabin commented 2 months ago

This means any hooks that plugins might add are added too late, and none is executed on the first slide.

Some results of my investigation

The first bad commit is this: https://github.com/LeaVerou/inspire.js/commit/60e038c8eafe64e48a2f49dc19db3f95386c83a0

In this commit, the way the plugins are loaded was changed.

The first run of the slidechange hook happens here: https://github.com/LeaVerou/inspire.js/blob/4562d780ca750b7d1ba94f007203b5cdc33ca9c1/src/inspire.js#L442

However, at this moment, none of the plugins added their code to the Hooks object.

Even though the process of loading plugins was initiated, https://github.com/LeaVerou/inspire.js/blob/4562d780ca750b7d1ba94f007203b5cdc33ca9c1/src/inspire.js#L50 none was actually loaded (we can see it from this.dependencies, which is an empty array).

SCR-20240621-qlce

This code https://github.com/LeaVerou/inspire.js/blob/4562d780ca750b7d1ba94f007203b5cdc33ca9c1/src/plugins.js#L58 is executed after the init process is finished. So, all plugins registered their hooks after the init process.

SCR-20240621-qlce

This means none of the plugins that rely on hooks don’t work on the first slide (not only the slide-style plugin).

To see the bug in action

(from Lea)

Visit the main demo and notice how the cover is purple. This is because a certain scoped style on a slide makes its background purple. If you go to the next slide and then back, the slidechange event runs, and the cover slide looks as it should (the hooks start working).

DmitrySharabin commented 2 months ago

Another thing I noticed is that here https://github.com/LeaVerou/inspire.js/blob/f50e735a2a4f1038097e4bc0fda1d4b81175fd7c/src/inspire.js#L54 is expected that this.dependencies is an iterable of promises. However, even though this.dependencies is an iterable (an array), its elements are not promisesthey are objects whose properties are promises: https://github.com/LeaVerou/inspire.js/blob/f50e735a2a4f1038097e4bc0fda1d4b81175fd7c/src/plugins.js#L19-L41

Does Promise.allSettled() work in this case?

DmitrySharabin commented 2 months ago

If we fix the code so that this.dependencies will be an array of promises, the code execution will freeze on this line—the promise corresponding to the live-demo plugin will never be resolved because on this line: https://github.com/LeaVerou/inspire.js/blob/f50e735a2a4f1038097e4bc0fda1d4b81175fd7c/plugins/live-demo/plugin.js#L126

The plugin awaits slideshowCreated to be resolved. However, it happens in init() which is called in setup() https://github.com/LeaVerou/inspire.js/blob/f50e735a2a4f1038097e4bc0fda1d4b81175fd7c/src/inspire.js#L62 after all the plugins should be loaded (and the corresponding promises should be resolved):

This is the line where the slideshowCreated resolves: https://github.com/LeaVerou/inspire.js/blob/f50e735a2a4f1038097e4bc0fda1d4b81175fd7c/src/inspire.js#L295

I submitted a PR with a possible fix: #162

@LeaVerou, could you please take a look?