First iteration of performance tweaks #27
Conversation
|
Don't merge this now, please. I want to add some tests. P.S.: But I'll be glad if you look your standards. |
| @@ -2,5 +2,7 @@ language: node_js | |||
| sudo: false | |||
There was a problem hiding this comment.
language: node_js
node_js:
- node
- 6
- 4
cache:
directories:
- node_modules
before_script:
- npm i
script:
- npm test
after_success:
- './node_modules/.bin/nyc report --reporter=text-lcov | ./node_modules/.bin/coveralls'
notifications:
email: false
| // Since we are matching multiple sets of delimiters, we need to run a loop | ||
| // here to match each one. | ||
| for (let i = 0; i < settings.length; i++) { | ||
| const matchs = input.match(settings[i].regexp) |
There was a problem hiding this comment.
typo ? matchs = matches
| options = Object.assign({ | ||
| delimiters: ['{{', '}}'], | ||
| unescapeDelimiters: ['{{{', '}}}'], | ||
| conditionalTags: ['if', 'elseif', 'else'], |
There was a problem hiding this comment.
Are options for setting the conditional-, loop-, scope tags really necessary? what if someone tries to add e.g options.loopTags = ['each', 'foreach', 'forof', 'forin'...] expecting them to work like the normally do in JS ? :) To reduce eventual confusion and configuration imo, we should maybe consider removing them from options enterily. It's not really a benefit being able to customize them :). But maybe I'm missing something important? cc @voischev @posthtml/collaborators
There was a problem hiding this comment.
In original code (https://github.com/posthtml/posthtml-expressions/blob/master/lib/index.js#L129) uses only the first element for loop & scope.
There was a problem hiding this comment.
Ok I see, that's 👍 for dropping I would say :). Fixed tags for logical expressions, matching the JS naming conventions && logic when possible, is a valuable thing. Everyone can still use his/her's favourite delimiter flavour of choice. But not my decision alone, lets wait for input of others :)
|
@mrmlnc Whats your opinion on a <switch>
<case>
<case>
<case>
</switch>likely this one is total rubbish 😛 , anyways...
|
|
I want to talk about it soon. Because all these changes (a needle in a haystack) do not solve the problem, which was announced in #26. |
|
Are there no known faster implementations of |
|
Spoiler: even a simple |
|
uhhhh... 😟 I'm an extensive |
|
Ready to merge. Please look again 😄 |
|
@mrmlnc I make few chore/docs tweaks later today and publish new release under |
| delimiters.push('escaped') | ||
| unescapeDelimiters.push('unescaped') | ||
| function escapeRegexpString (input) { | ||
| return input.replace(/[|\\{}()[\]^$+*?.]/g, '\\$&') |
There was a problem hiding this comment.
should be use cached variable for RE
There was a problem hiding this comment.
Will be resolved by performance-2
| @@ -278,7 +276,7 @@ function executeLoop (loopParams, p1, p2, node) { | |||
| */ | |||
| function executeScoped (scopedLocals, node) { | |||
| // merge nondestructively into existing locals | |||
| const scopedOptions = merge(cloneDeep(options), { locals: scopedLocals }) | |||
| const scopedOptions = merge(cloneDeep(data), { locals: scopedLocals }) | |||
| const vm = require('vm') | ||
|
|
||
| function escapeHtmlCharacters (unsafe) { | ||
| return unsafe.replace(/[&<>"']/g, (match) => ({ |
There was a problem hiding this comment.
Object and RE should be cached ;)
There was a problem hiding this comment.
Will be resolved by performance-2
Proposed Changes
Replacing the placeholders
string.replace2-2.5x faster thanvm.runInContext, so we can usestring.replacefor simple expression (without dynamic calculations).If the expression has something besides letters and numbers - use VM otherwise replace.
Benchmark [150x
{{ key }}]Loop statement parser
Use new solution for parsing loop statement
Benchmark
Manual expansion the expression of conditions
Benchmark [20x
if - else if -else if - else]Types of Changes
Fully compatible with the current version.