PR2492 causes regression to Modal

[yiwen101]

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

No response

Tell us about your environment

MacOs

MarkBind version

Current master branch

Describe the bug and the steps to reproduce it

After #2492, modal cannot pop up properly. Can test with this:

More about <trigger for="modal:loremipsum">trigger</trigger>.
<modal header="**Modal header** :rocket:" id="modal:loremipsum">
  Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore
  magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
  consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
  Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
</modal>
<br>
This is the same <trigger for="modal:loremipsum">trigger</trigger> as last one.

<trigger for="modal:centered">This is a trigger for a centered modal</trigger>.

<modal header="**Centered** :rocket:" id="modal:centered" center>
  Centered
</modal>

<trigger for="modal:ok-text">This is a trigger for a modal with a custom OK button</trigger>.

<modal header="OK button visible!" id="modal:ok-text" ok-text="Custom OK">
  Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore
  magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo
  consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
  Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
</modal>

Expected behavior

No response

Anything else?

No response

[damithc]

Thanks for reporting this @yiwen101 If confirmed, need to fix quickly, as we can't do another release until this is fixed.

[yucheng11122017]

Will revert the PR first. @LamJiuFong would be good if you have the bandwidth to figure out the issue and if its possible to fix.

[LamJiuFong]

@yucheng11122017 I am looking at it now, thanks

[LamJiuFong]

This was the exact issue:

I believe the issue occurred due to the dependencies between this script and the deferred scripts:

Once the scripts in the second image are deferred, they are executed later than the first one, hence there is a reference error.

Solution: We have to make sure that the four scripts are being executed before the first one, in this case, we cannot use defer because i. deferred scripts are loaded last (as stated above) ii. While defer preserves execution order, adding defer to the first script component still cannot work because defer only works on <script> with a src attribute.

However, we can still move the scripts to the bottom (have to make sure they are above the first script). It can still save rendering time as the files are executed after all DOM elements are loaded.

[yucheng11122017]

This was the exact issue:

I believe the issue occurred due to the dependencies between this script and the deferred scripts:

Once the scripts in the second image are deferred, they are executed later than the first one, hence there is a reference error.

Solution: We have to make sure that the four scripts are being executed before the first one, in this case, we cannot use defer because i. deferred scripts are loaded last (as stated above) ii. While defer preserves execution order, adding defer to the first script component still cannot work because defer only works on <script> with a src attribute.

However, we can still move the scripts to the bottom (have to make sure they are above the first script). It can still save rendering time as the files are executed after all DOM elements are loaded.

Thanks for the quick investigation @LamJiuFong!

Is it possible to change the first script to use a src attribute?

Could you investigate on the performance difference when pushing the scripts to the bottom vs our current implementation?

[LamJiuFong]

@yucheng11122017 thanks for the suggestions, I have tried them out:

Could you investigate on the performance difference when pushing the scripts to the bottom vs our current implementation?

On my end, pushing the scripts to the bottom takes ~17s to load the DOM content while the current implementation takes ~20s to load

Is it possible to change the first script to use a src attribute?

I think this will be tricky as we have to add js files as one of the asset of a PageConfig when creating new pages in Site/index.ts::createPage, it would probably involve some refactoring of code

Perhaps just moving the scripts to the bottom is enough, what do you think?

[kaixin-hc]

@LamJiuFong I think it is fine to move the scripts to the bottom to fix the OG issue; then we can change this issue to be about exploring refactoring / making it possible to use defer, with more investigation.