-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
better maintainability #335
base: main
Are you sure you want to change the base?
better maintainability #335
Conversation
classes instead of prototype
Did you run benchmarks? Splitting into multiple files means 3 more accesses to the file system. |
I don't see how this would give us better maintainability. |
My point is that it will be more convenient for people to read the library It seems to me that when everything is in one file and written in a relatively old format, it is very difficult to maintain |
|
I'm sure it didn't affect the benchmark because the library is imported there 1 time But if you're really worried about importing multiple files, you can add a build that will put everything into 1 file without hurting the developer experience |
Looks like there are some regressions. |
I'm sure that you can't rely on this benchmark, since you can take 30% difference in favor of PR (when nothing logically changed) node is not accurate enough to see the difference in 20988 ns (difference * sampled) for 574 samples |
I believe that such quick tests have values on a logarithmic scale |
I repeated the benchmark for the
So I can say that several tests work better in the morning than in the evening 😅 |
Sorry, but I can't find a way to test these quick functions
in any case, after several runs of the same benchmark, I was getting more than 10% difference |
Do you have any comments on PR? |
@ivan-tymoshenko @delvedor wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm for me
@@ -0,0 +1,15 @@ | |||
const isRegexSafe = require('safe-regex2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'use strict'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how it improves maintainability, but I'm not against it.
@@ -0,0 +1,567 @@ | |||
const assert = require('assert') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'use strict'
lib/utils.js
Outdated
@@ -0,0 +1,70 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
separate files
classes instead of prototype