Upgrade js-beautify and provide option to turn it off #1116

yash-chowdhary · 2020-03-10T16:47:29Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Enhancement to an existing feature

Resolves #1067

What is the rationale for this request?
We use an old version of js-beautify (v1.7.5 as opposed to the latest v1.10.3).
As documented in #1067, js-beautify affects the performance of our build process. It would be beneficial for MarkBind to turn off js-beautify, at the same time allowing users to choose to turn it on if needed.

What changes did you make? (Give an overview)

Upgrades js-beautify to v1.10.3
Adds support for a jsBeautify option in site.json, which when set to true will make use of js-beautify on HTML files

Provide some example code that this change will affect:
Added a check in multiple places similar to this -

this.content = this.jsBeautify 
               ? htmlBeautify($.html(), Page.htmlBeautifyOptions)
               : $.html();

Is there anything you'd like reviewers to focus on?
Since I've updated the js-beautify library, there have been changes in the expected output of the test sites according to the new changes/rules added in the library.
Also, the test_site_convert and test_site_template run init, which means they will have js-beautify turned off. Just a heads up in case anyone was wondering why the formatting is off for the HTML files in those test sites.

Testing instructions:
npm run test

Proposed commit message: (wrap lines at 72 characters)

Upgrade js-beautify and provide users with an option to toggle it on.

MarkBind currently runs js-beautify v1.7.5, although later versions
of the library have been released (the latest version as of this PR
is v1.10.3).

Turning off js-beautify can improve the performance of MarkBind's 
build process, so let's make that the default setting.

Let's upgrade js-beautify to the latest version.
Let's also give the user the option to turn on js-beautify (whenever
necessary) through an option in site.json.

yash-chowdhary · 2020-03-11T11:14:15Z

@ang-zeyu can I get your review on this?

ang-zeyu

A quick look:

docs/userGuide/siteConfiguration.md

package.json

src/Page.js

src/Site.js

src/Page.js

test/functional/test_site/expected/test_md_fragment.html

docs/userGuide/siteConfiguration.md

test/functional/test_site/expected/testAnchorGeneration.html

ang-zeyu

LGTM 👍

@ further reviewers there's quite a few changes to test_site_convert and test_site_template, but its expected since htmlBeautify is off for those. Might be nice to have a mix of test sites with this option on / off as well. If it wasn't broken after htmlBeautifying it before, it shouldn't be broken by turning off htmlBeautify either. ( also since htmlBeautify is the final step in page generation )

Some miscellaneous changes were observed for the other test sites with htmlBeautify turned on otherwise which are expected due to the bug fixes / changes in htmlBeautify.

marvinchin

I would prefer to default to enabling htmlBeautify (rationale in comments). As always though, if anyone has differing opinions on this I'm happy to discuss 🙂

marvinchin · 2020-03-26T15:55:41Z

docs/userGuide/siteConfiguration.md

@@ -199,6 +199,18 @@ The example above uses tags as an example of configuring plugin settings, refer

 **Specifies that the website should use MarkBind's search functionality.** Default: `true`. See [User Guide: Making the Site Searchable](makingTheSiteSearchable.html) for more details.

+#### **`htmlBeautify`**
+
+**Toggles-on html beautification done by [js-beautify](https://github.com/beautify-web/js-beautify) for html files generated by MarkBind.** By default, MarkBind turns this option off to improve the performance of the MarkBind `build` process. 


I personally lean towards defaulting to having htmlBeautify on, and allowing users to turn it off if they really wish to get the performance improvement.

The reason behind this are:

The build process generally isn't very time critical or performance sensitive for most use cases

Standardisation of our generated files make it easier to ignore diffs for non-material changes (i.e. markbind might make changes internally that changes the indendentation of files, but these changes don't actually change the markup of the site)

Having inconsistently formatted files by default isn't really a good look for the product

The build process generally isn't very time critical or performance sensitive for most use cases

Hmm, although there's markbind serve -o now, it could still really help when writing the page ( ~20% performance gains ).

Having inconsistently formatted files by default isn't really a good look for the product

Good point though

Standardisation of our generated files make it easier to ignore diffs for non-material changes (i.e. markbind might make changes internally that changes the indendentation of files, but these changes don't actually change the markup of the site)

The test sites which don't have htmlBeautify turned on are only the template sites and test_site_convert.

In this sense, how about changing the one for the template sites to be on instead? This should solve the problem of presenting a good first impression, while defaulting to off if it is not specified.

test_site_convert would be an acceptable compromise here I think, but a future pr could always tackle this 😄

Hmm, although there's markbind serve -o now, it could still really help when writing the page ( ~20% performance gains ).

Good point. I wonder what the performance cost for turning beautify on is. Would anyone happen to have some metrics on that?

In this sense, how about changing the one for the template sites to be on instead? This should solve the problem of presenting a good first impression, while defaulting to off if it is not specified.

I think that the build assets that we generate for the user matters as well. For the typical user, build performance is probably not a big concern. Users who are performance conscious are also generally more willing to tinker with their tools. But this, of course, is difficult to know for certain without further user research :P

I feel that the defaults should be set in a way such that a typical user needs minimal configuration to get an output they are happy with. In this case, it seems like focusing on user friendliness might make more sense than optimizing for performance performance.

Maybe as part of user research we can ask @damithc to see if he happens to have any thoughts on this? 😃

Good point. I wonder what the performance cost for turning beautify on is. Would anyone happen to have some metrics on that?

Yup, its in the original issue #1067

I think that the build assets that we generate for the user matters as well. For the typical user, build performance is probably not a big concern.

Wouldn't adding htmlBeautify: on to the template solve this though? ( since these are the sites the user starts with during `markbind init )

I feel that the defaults should be set in a way such that a typical user needs minimal configuration to get an output they are happy with.

This does feel cleaner, my worry here is that performance of markbind serve also affects ux ultimately though. Not too sure which of the two we should go with either 😬

Yup, its in the original issue #1067

Thanks for pointing me to it :)

Wouldn't adding htmlBeautify: on to the template solve this though? ( since these are the sites the user starts with during `markbind init )

Ah okay, I see what you mean there. I still think that it makes logical sense for the default behaviour and the default configs we provide to be aligned, but changing the template site.json might be an acceptable compromise.

@damithc could we get your opinion on whether performance / neatness of generated source files should be prioritised in the default behaviour?

Perhaps safer to prioritize neatness as basic users might not need the performance gain (which will be tiny for small web sites) but might get a shock if they to take a look at the generated code? I am OK to go either way though.

In that case, we can shift to enabling it by default with an option to turn it off.

marvinchin · 2020-03-26T15:57:29Z

src/Site.js

@@ -922,7 +923,7 @@ class Site {
    injectMarkdownItSpecialTags(tagsToIgnore);
    Page.htmlBeautifyOptions = {
      indent_size: 2,
-      content_unformatted: ['pre', ...tagsToIgnore],
+      content_unformatted: ['pre', 'textarea', ...tagsToIgnore],


What's the rationale for adding this new textarea option?

html beautify was updated with this as well since our last version; unfortunately the options don't merge with the defaults, so a little manual inspection has to be done here

https://github.com/beautify-web/js-beautify/blob/master/js/src/html/options.js

ang-zeyu

Needs an update on master, otherwise just one nit!

docs/userGuide/siteConfiguration.md

ang-zeyu

LGTM 👍

* 'master' of https://github.com/MarkBind/markbind: (41 commits) Document adding new site content in DG (MarkBind#1153) Add relative date feature (MarkBind#908) Use <br> to separate lines of code block (MarkBind#1176) Parse popovers for footnotes (MarkBind#1155) Resolve comments Expand test extensions and fix whitespace checks (MarkBind#1156) Address comments Upgrade js-beautify and provide option to turn it off (MarkBind#1116) Normalize inline puml line ending before hashing (MarkBind#1174) Update tests (MarkBind#1178) Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148) Fix bug in Search for UG and DG (MarkBind#1147) Add inline puml support (MarkBind#1100) Fix hrefs for headings with angular brackets (MarkBind#1089) Update tests for 2.13.1 (MarkBind#1169) 2.13.1 Update vue-strap version to v2.0.1-markbind.39 Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133) 2.13.0 Update test files ...

MarkBind currently runs js-beautify v1.7.5, although later versions of the library have been released (the latest version as of this PR is v1.10.3). Let's upgrade js-beautify to the latest version. Turning off js-beautify can improve the performance of MarkBind's build process. Let's also give the user the option to turn off js-beautify (whenever necessary) through an option in site.json.

It seems like the changes to the expected test files in #1116 were overwritten by later PRs. Let's update the test files so that they are formatted by js-beautify again.

Previously in 8a5c42c we updated the test files because we thought that the changes from #1116 were overwritten by later PRs. However, it turned out that the local node modules was not updated after pulling from master, hence the original changes were correct. Let's revert the changes to the test files.

yash-chowdhary changed the title ~~Upgrade beautify and provide option to user to toggle it on~~ Upgrade js-beautify and provide option to user to toggle it on Mar 10, 2020

ang-zeyu reviewed Mar 11, 2020

View reviewed changes

yash-chowdhary force-pushed the upgrade-beautify branch from ecf7a68 to a4e186d Compare March 20, 2020 12:31

ang-zeyu reviewed Mar 20, 2020

View reviewed changes

test/functional/test_site/expected/test_md_fragment.html Outdated Show resolved Hide resolved

docs/userGuide/siteConfiguration.md Outdated Show resolved Hide resolved

ang-zeyu reviewed Mar 21, 2020

View reviewed changes

test/functional/test_site/expected/testAnchorGeneration.html Show resolved Hide resolved

yash-chowdhary force-pushed the upgrade-beautify branch from 7778f34 to da56974 Compare March 23, 2020 06:32

yash-chowdhary requested a review from ang-zeyu March 23, 2020 06:40

yash-chowdhary force-pushed the upgrade-beautify branch from 4ead9a6 to e7457bc Compare March 23, 2020 08:25

ang-zeyu approved these changes Mar 23, 2020

View reviewed changes

ang-zeyu requested a review from marvinchin March 26, 2020 15:28

marvinchin reviewed Mar 26, 2020

View reviewed changes

yash-chowdhary added the pr.Enhancement 📈 Enhancement to an existing feature label Apr 1, 2020

yash-chowdhary changed the title ~~Upgrade js-beautify and provide option to user to toggle it on~~ Upgrade js-beautify and provide option to turn it off Apr 2, 2020

yash-chowdhary force-pushed the upgrade-beautify branch from 6bf388e to d9628ff Compare April 2, 2020 14:20

ang-zeyu reviewed Apr 2, 2020

View reviewed changes

docs/userGuide/siteConfiguration.md Outdated Show resolved Hide resolved

yash-chowdhary added 11 commits April 3, 2020 09:55

Upgrade beautify and support option to toggle js-beautify

5200f08

Update test sites

862020f

Update user guide

cbb1456

Update js-beautify; refactor and remove redundancies

ce5c514

Update test site beautification and docs

4645919

Update docs

b66571d

Update functional tests

90a8220

Resolve conflicts

cf529b6

Beautify by default

703b237

Resolve conflicts

bcff35d

Update documentation

47c9443

yash-chowdhary force-pushed the upgrade-beautify branch from d9628ff to 47c9443 Compare April 3, 2020 02:02

ang-zeyu approved these changes Apr 3, 2020

View reviewed changes

ang-zeyu added this to the v2.13.2 milestone Apr 3, 2020

yash-chowdhary requested a review from marvinchin April 4, 2020 07:09

marvinchin merged commit d6bf210 into MarkBind:master Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade js-beautify and provide option to turn it off #1116

Upgrade js-beautify and provide option to turn it off #1116

yash-chowdhary commented Mar 10, 2020 •

edited

Loading

yash-chowdhary commented Mar 11, 2020

ang-zeyu left a comment

ang-zeyu left a comment

marvinchin left a comment

marvinchin Mar 26, 2020

ang-zeyu Mar 26, 2020

marvinchin Mar 26, 2020 •

edited

Loading

marvinchin Mar 26, 2020

ang-zeyu Mar 26, 2020

marvinchin Mar 26, 2020

ang-zeyu Mar 27, 2020

damithc Mar 27, 2020

yash-chowdhary Mar 28, 2020

marvinchin Mar 26, 2020

ang-zeyu Mar 26, 2020

ang-zeyu left a comment

ang-zeyu left a comment

Upgrade js-beautify and provide option to turn it off #1116

Upgrade js-beautify and provide option to turn it off #1116

Conversation

yash-chowdhary commented Mar 10, 2020 • edited Loading

yash-chowdhary commented Mar 11, 2020

ang-zeyu left a comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

marvinchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marvinchin Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

yash-chowdhary commented Mar 10, 2020 •

edited

Loading

marvinchin Mar 26, 2020 •

edited

Loading