refactored insertIntoIndexHTML#151
Conversation
| if (body) { | ||
| let isBodyReplaced = false; | ||
| html = html.replace("<!-- EMBER_CLI_FASTBOOT_BODY -->", function() { | ||
| html = html.replace(/<\!-- EMBER_CLI_FASTBOOT_(HEAD|BODY) -->/g, function(match, p1) { |
There was a problem hiding this comment.
My two cents:
I really find the changes here convoluted and the head and body fork is at two places instead of one - making readability hard. I am not convinced this is improving readability if that is what the motivation of the changes was.
There was a problem hiding this comment.
motivation is was improve performance :P, one .replace is better than .replaces, simple benchmark I used https://gist.github.com/bekzod/f08628db3cbbbd678bbc8f72e4b01001 , readability-wise I would say it is same level as before at least it uses less lines of code
There was a problem hiding this comment.
By how much was the performance improved?
There was a problem hiding this comment.
before change: 370ms
after change: 193ms
so 50%
There was a problem hiding this comment.
at least on my machine (mac os node v6.8.1)
| if (body) { | ||
| let isBodyReplaced = false; | ||
| html = html.replace("<!-- EMBER_CLI_FASTBOOT_BODY -->", function() { | ||
| html = html.replace(/<\!-- EMBER_CLI_FASTBOOT_(HEAD|BODY) -->/g, function(match, p1) { |
There was a problem hiding this comment.
can we use a better variable than p1?
| if (!isHeadReplaced) { | ||
| return missingTag('<!--EMBER_CLI_FASTBOTT_HEAD-->'); | ||
| } | ||
| if (head && !isHeadReplaced) { |
There was a problem hiding this comment.
Why check for head and body here? Seems unecessary since it defaults to empty string.
There was a problem hiding this comment.
https://github.com/bekzod/fastboot/blob/45087528437aa0d1c2dc620b9a45898a8063ce1a/src/result.js#L37-L39 because sometimes missing head insert comment tag is still valid, the same logic was used before, if I remove head bunch of test are broken
There was a problem hiding this comment.
But body will always be present. It seems an unecessary check. But I am fine either ways
bekzod
force-pushed
the
refactor-insert-html
branch
from
4508752 to
d238901
Compare
2 participants
No description provided.