next-theme / pjax

Easily enable fast Ajax navigation on any website (using pushState + xhr)
MIT License
8 stars 2 forks source link

Fix script execute order #6

Closed PaperStrike closed 3 years ago

PaperStrike commented 3 years ago

PR Type

What is the current behavior?

  1. Scripts execute in wrong order when external and inline ones mixing together.

    Setting async to false will only make external scripts be executed in order. Inline scripts, as the standard describes, will be executed immediately when inserted. Pjax hasn't write something for the mix of them.

  2. Dependencies not up to date.

  3. Code style not fit NexT one.

What is the new behavior?

  1. Execute them in right order.

    There are three methods I considered. Edit: See https://github.com/next-theme/pjax/pull/6#issuecomment-830816632 for newest solution.

    1. Insert the scripts in the page one-by-one. Insert next script only when the previous one has been executed.
    2. Insert external scripts first. Insert inline ones after the last external script got executed.
    3. Slice the scripts in the page into groups. Each group has no more than one external script. If a group has a external script, the external script must be the first element in the group. For each group, insert the scripts one-by-one. Insert next script only when the previous one in the group has been executed.

    The first method is easy but it will slow the fetch. The second may solve some problems, but doesn't contribute to a right order strictly speaking. For the third method, it doesn't affect the parallel fetch and contributes to a right order.

    A fetch flow graph

    (The fetch flow graph in a test)

    I was worried about the execute order between groups. Fortunately, the callback of a external script's load event is called before the next external script can be executed. Appearently I decide to use the group method. In tests, it behaves almost the same as the initial loading of the page.

  2. Update dependencies.

  3. Use @next-theme/eslint-config to lint JS files.

Other information

Browser Compatibility

Browser Supported versions Release date Limitation
IE N/A N/A N/A
Edge 17+ Apr 30, 2018 DOM manipulation convenience methods
Firefox 50+ Nov 15, 2016 NodeList API: forEach
Chrome 54+ Oct 12, 2016 DOM manipulation convenience methods
Safari 10+ Sep 20, 2016 Arrow functions
Opera 41+ Oct 24, 2016 DOM manipulation convenience methods
PaperStrike commented 3 years ago

Execute order is still a problem in the group method. Imagine an external script B, which is supposed to be evaluated later than A, got a fetch error before A is fetched - the other scripts in the group of B get evaluated ealier.

So I keep digging and got a fourth method. It doesn't group the blocking scripts, but combine or chain the evaluate promise according to whether the script is external.

https://github.com/next-theme/pjax/blob/818b9b1413ac2fd2818b857e8e7a131936317da2/lib/execute-scripts.js#L19-L28

This approach seems to have the same advantage as the group method while providing a more reliable execute order.

PaperStrike commented 3 years ago

This PR focuses on the execute order. ES6 refactor will continue in future PR. ESLint fails currently.

PaperStrike commented 3 years ago

Close as the maintain difficulty doesn't match its importance in NexT. Continued in PaperStrike/Pjax.