add the backslash support #12
Conversation
Pull Request Test Coverage Report for Build 44
💛 - Coveralls |
|
em... not sure why build is failing on old version of node. I haven't change the package.json file and couldn't reproduce it either locally. I'll try to resolve it |
|
i try to build it again and it passes with one restart. https://travis-ci.org/meetall/nimnjs-node could you please restart the build for the failed node versions? thanks |
src/decoder.js
Outdated
|
|
||
| for(;this.index < len && until.indexOf(this.currentChar()) === -1;this.index++); | ||
| for(;this.index < len; this.index++){ | ||
| if(this.currentChar() === '/' && this.index+1 < len |
src/encoder.js
Outdated
| function sanitizeData(data) { | ||
| if(typeof data === "string"){ | ||
| appCharsArr.forEach(c => { | ||
| data = data.replace(new RegExp(c, 'g'), '/'+c); |
|
yes, I'll update the forward slashes to backslash. Thanks for spotting. For the single quote and double quote issue, I thought both works in javascript? But I can change them to double quote if you want to keep some convention in the code. |
|
@meetall Umm no actually git has removed backslash char from my comment. I wanted to say Using single quote is better but double quote will also work. So that's not the big problem. |
|
@amitguptagwl Updated now |
|
My apologies for the delay. But I'm quite busy with some other tasks. I'll do it ASAP. |
src/encoder.js
Outdated
| function sanitizeData(data) { | ||
| if(typeof data === "string"){ | ||
| appCharsArr.forEach(c => { | ||
| data = data.replace(new RegExp(c, 'g'), '\\'+c); |
There was a problem hiding this comment.
I believe this loop can be avoided with range match.
src/encoder.js
Outdated
| appCharsArr.forEach(c => { | ||
| data = data.replace(new RegExp(c, 'g'), '\\'+c); | ||
| }) | ||
| let regex = new RegExp(appCharsArr.join('|'), 'g'); |
There was a problem hiding this comment.
Do we need to create the regex on every function call? I believe it'll impact the performance too.
|
np, I'll have a look. thanks
…On Thu, 7 Jun 2018, 04:33 Amit Gupta, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Sorry for all the revisions but performance is one of the important
concerns of this project.
Can you please check if there is any other change which may impact the
performance?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD7s72_k2lCKMCVQ6othdPzA_SWKfpY6ks5t6J8SgaJpZM4UQju3>
.
|
|
Hello, I made the update you suggested and did some research as well. But couldn't figure out any other improvement. The actual code change in this PR is quite minor (basically "replace" and "substr"). I can see you have another issue in the backlog to improve the algorithm. Maybe it could be re-evaluated as a whole. I also looked into some javascript profiling tool but all of them are for node web applications. Do you have any plans to add performance test if it is a critical factor? |
|
Performance is really important. Hence I'm rewriting the whole algorithm. I'm almost finished and will publish it tomorrow. Let's see how much we achieve. |
Backslash characters #8