Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile Vue Page at build time to alleviate FOUC #1512

Merged

Conversation

wxwxwxwx9
Copy link
Contributor

@wxwxwxwx9 wxwxwxwx9 commented Mar 10, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Addresses the first part of #1406 by compiling our page (Vue App) during build time and thus eliminating FOUC for majority of the use-cases.

Overview of changes:

The approach has largely changed. Refer to the discussion in the reviews below.

Explanation of Approach
During the generation of the page and after the relevant processings of the page are finished, I introduced a new step of compiling the page into a Vue Application using vue-template-compiler. The compilation returns me with an object that contains the relevant information about the rendered Vue app. Only 2 attributes are important: render and staticRenderFns. These are the render functions provided by Vue that will be used to generate the view in the browser.

After getting these 2 things, I map them to the route of the page in an object. This object keeps a mapping of a page's route (e.g. /index.html) to its respective render function, so that we can pass in this render function to Vue during mounting to render the correct page view according to the current page route.

This object will then be output into a javascript file, pageVueRenderFns.min.js, that will be sent along with the rest of the assets to the browser so that our MarkBind plugin (core-web) can access the object, retrieve the appropriate render function according to the current page route, and pass the render function into the Vue instance during mounting.

Technically, with the above, we no longer need to insert the page content into our page.njk template. This is because we have already compiled the content into the render function. However, it is still necessary to insert the page content into the Nunjucks template because we have to keep track of the DOM structure for snapshot testing.

This also means that we will have to send the uncompiled page along with index.html to the browser. However, this is undesirable as we do not want the uncompiled version of the page to be rendered (this will cause FOUC and defeat the purpose of pre-compiling in the first place). Thus, I introduced another script, initAppNodeForPageVueRender.min.js, and placed it before the uncompiled page to delete it before it can be rendered. The script will also create a fresh #app node for Vue to mount, using the render function we have from pre-compiling.

Removal of the interim fix for FOUC
Due to the resolution of the FOUC problem, we no longer need the interim fix provided by tempStyleProcessor. Thus, I have deleted all the relevant code and the file itself.

By removing the interim fix, this PR will also address the dropdown issue from #1472.

Adding type="application/javascript" attribute to script tags
By default, Vue compilation ignores and discards any script nodes during its parsing and compilation. It will provide the following error/warning message.

Error: Templates should only be responsible for mapping the state to the UI. Avoid placing tags with side-effects in your templates, such as <script>

I noticed that we have some use-cases for script tags in our source files currently. For example, in the index.md of our UserGuide, we will redirect the user with the following script tag:

<script>
  window.location.href = "gettingStarted.html"
</script>

If we do not add the attribute type="application/javascript" to the script tag, the above will be discarded during compilation. Thus, I introduced a new processing step for script nodes to add the attribute. I don't think this will affect any existing features or cause any regression. At least I have not seen any so far. But let me know if this is something that may be undesirable :-)

Changing <style> tags name to workaround Vue compilation discard
Vue compilation discards any <style> tags, similar to <script>. But <style> tags does not have a straightforward solution like <script> (read above). We have to change the name of style tag into a placeholder name for Vue compilation so that Vue doesn't discard the element. We will then change the name back to style tag after compilation.

Anything you'd like to highlight / discuss:

In general:

  • Whether there is any problem with my approach or anything that was mentioned above
  • Ways to improve the implementation
  • Any potential bugs that may be introduced by the changes that I should look into
  • Whether it will affect any of the existing features or implementations

Also, I have currently commented out the code where I minified pageVueRenderFns.min.js and initAppNodeForPageVueRender.min.js using UglifyJs, as I did not notice a significant reduction in file size (from 4.3MB to 4MB only for pageVueRenderFns.min.js). Just wanted to check if it's worth using the library considering that there's not much reduction?

Testing instructions:
npm run test

Serve a sizable MarkBind project (e.g. our documentation) and you should see that the FOUC doesn't occur anymore for the navbar.

Proposed commit message: (wrap lines at 72 characters)
Compile Vue Page at build time to alleviate FOUC.

Compiling each Vue page in the browser may be expensive as it is
possible for a page to get very large in size. This creates the problem
of flash-of-unstyled-content (FOUC) for the user which is unsightly
and may affect user experience. Although there are temporary styles
in place to tackle the problem, the FOUC problem largely remains.

Let's compile every Vue page during build time to alleviate FOUC.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@jonahtanjz
Copy link
Contributor

jonahtanjz commented Mar 11, 2021

@wxwxwxwx9 Good work on this PR 👍

There seems to be an issue with rendering some of the pages such as User Guide and Developer Guide. It is currently showing a blank page.

After looking around, it seems the pageVueRenderFns in core-web is not loading the updated version. It only contains / and /index.html from what I can see after visiting different pages.

Another part that I think might have an issue is the pagePath in the compileVuePageAndMapRenderFn function. The relative path on Windows will use backslash (eg. userGuide\index.html) whereas window.location.pathname will use forward slash. This will result in a different path which will cause pageVueRenderFns[pageRoute] to return undefined.

@wxwxwxwx9
Copy link
Contributor Author

@ang-zeyu I have made the changes as per your suggestions :-) let me know if there's anything else I should work on!

packages/core-web/src/index.js Outdated Show resolved Hide resolved
packages/core-web/src/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the workaround 👍

Last few nits:

packages/core/src/Page/index.js Outdated Show resolved Hide resolved
packages/core/src/html/NodeProcessor.js Outdated Show resolved Hide resolved
packages/core/src/html/NodeProcessor.js Outdated Show resolved Hide resolved
.bigger-text {
font-size: 10em;
<script src defer type="application/javascript" style-bypass-vue-compilation>
.bigger - text {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

sorry, just noticed this as well. Could you investigate it? @wxwxwxwx9

Copy link
Contributor Author

@wxwxwxwx9 wxwxwxwx9 Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I noticed this as well but I figured it shouldn't be a cause of concern, since we are not running the script and the conversion back to <style> tag doesn't seem to have any problem. Do you think it may cause a problem?

EDIT: By the way, forgot to mention, it's caused by the htmlBeautify function.

Screen Shot 2021-03-19 at 10 59 31 PM

One possible way to prevent this is to use the ignore directive from js-beautify. I think there are some benefits to this since it does help to reduce build-time if we were to prevent this redundant beautifying work. I will work on this. Let me know what you think :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just js-beautify's content_unformatted option? though this means blanket ignoring all script tags which should be ok

Copy link
Contributor Author

@wxwxwxwx9 wxwxwxwx9 Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should work fine as well, but do you think we should put a notice for it in userGuide/siteJsonFile.html#disablehtmlbeautify accordingly? That <script> tags are not beautified by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep it in mind for now.

beautifying might not even make sense after ssr with the "gibberish" output (current purpose is to allow the author to figure out markdown nuances / etc. causing unexpected outputs)

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 👍

@ang-zeyu ang-zeyu merged commit 8b317c4 into MarkBind:master Mar 19, 2021
@ang-zeyu ang-zeyu added this to the v3.0 milestone Mar 19, 2021
@wxwxwxwx9 wxwxwxwx9 mentioned this pull request Apr 2, 2021
10 tasks
@ang-zeyu ang-zeyu mentioned this pull request Jul 18, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants