feat: refactor handlebars into template literals. #978
Conversation
|
Status update:
|
aorinevo
force-pushed
the
replace_handlebars
branch
from
ae0db0f
to
01db48b
Compare
|
Status update:
|
aorinevo
force-pushed
the
replace_handlebars
branch
from
c3f3dcd
to
4f63ec4
Compare
| @@ -15,3 +15,7 @@ | |||
| - [Changelog](https://hapi.dev/family/lab/changelog/) | |||
| - [Project policies](https://hapi.dev/policies/) | |||
| - [Free and commercial support options](https://hapi.dev/support/) | |||
|
|
|||
|
|
|||
| # Notes | |||
| @@ -1427,7 +1614,7 @@ describe('Reporter', () => { | |||
| '<td class="source"><div> while ( </div><div class="miss false" data-tooltip>value ) </div><div>{</div></td>']); | |||
| expect(output, 'missed original line not included').to.contains([ | |||
| '<tr id="while.js__14" class="miss">', | |||
| '<td class="source" data-tooltip> value = false;</td>']); | |||
| '<td class="source" data-tooltip> value = false;</td>']); | |||
There was a problem hiding this comment.
This fails with feature flag off but passes with feature flag on.
It is possible to wrap this in the feature flag as well, but not sure it is worth the effort (see #977 (comment)).
|
All tests pass and coverage is at 100% with feature flag on. There is one unit test that fails with feature flag off. While it is possible to change the test to be backwards compatible, it might not be worth the effort (see #977 (comment)). |
.labrc.js
Outdated
| @@ -0,0 +1,3 @@ | |||
| module.exports = { | |||
| enableTemplateStringHTMLReporter: 'true' | |||
| }; | |||
|
|
||
| exports.scripts = () => { | ||
|
|
||
| return `<script> |
| var links = document.querySelectorAll('#menu a') | ||
| , link; | ||
|
|
||
| for (var i = 0, len = links.length; i < len; ++i) { |
There was a problem hiding this comment.
| for (var i = 0, len = links.length; i < len; ++i) { | |
| for (var i = 0; i < links.length; ++i) { |
This seems like it would be slightly simpler.
There was a problem hiding this comment.
Property lookup for links.length each and every time through the loop is wasteful, as the value doesn't change during the execution of the loop. By storing the value on len var we avoid subsequent lookups.
There was a problem hiding this comment.
There was a problem hiding this comment.
Array lookups are O(C) so either one should be equivalent. The advice in the link shared is specifically with regards to property lookups, which have the potential for additional costs due to needing to look up the prototype chain if local lookup fails.
There was a problem hiding this comment.
Oops, I think I miunderstood. The idea is to make it a constant-time lookup rather than looking up the property on an Array object. The point I believe still stands though bc JS engines optimize for these sorts of things. E.g. see https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JIT_Optimization_Strategies.
There was a problem hiding this comment.
JIT compilation is eventually performant. First the JIT compiler needs to identify warm and hot code segments. The warm code segments get sent to a baseline compiler and reused by the compiler where possible. The hot code segments get sent to an optimizing compiler to be used until the assumptions under which the hot code was identified are invalidated at which point the optimized code is discarded (see https://blog.logrocket.com/how-javascript-works-optimizing-the-v8-compiler-for-efficiency/).
TL;DR there is a performance hit initially but indistinguishable for n sufficiently large, where n is the right hand of the for-loop conditional.
|
|
||
| function find (y) { | ||
| var i = headings.length | ||
| , heading; |
There was a problem hiding this comment.
Not sure what's happening here, bit oddly formatted.
| var i = headings.length | ||
| , heading; | ||
|
|
||
| while (i--) { |
There was a problem hiding this comment.
I believe this would exit when i === 0, does headings[0] not need to be processed?
There was a problem hiding this comment.
0 does get processed. The while Evals then the -1 is done
Co-Authored-By: Jugal S <jugaltheshah@users.noreply.github.com>
|
Closing this PR for now as the vulnerability was addressed through a combination of PRs to Handlebars: |
Changes
handlebardswith natively supported template strings in a backwards compatible fashion.enableTemplateStringHTMLReporterto'true'in.labrc.js.Notes
hapijs:masterbranch consists of 95.5% javascript, 3.8% html, and 0.7% other.aorinevo:replace_handlebarsbranch consists of 95.5% javascript, 3.9% html, and 0.7% other.package.jsononce/if backwards compatibility is dropped.