rewrite the entire module #80
Conversation
nlf
force-pushed
the
nlf/rewrite
branch
2 times, most recently
from
446aeda
to
a090eeb
Compare
all internals have been rewritten to maintain a similar contract but to remove excessive use of regular expressions, unnecessary loops, the custom string templating engine, and various other bits of complexity BREAKING CHANGE extending with custom providers has changed
|
I'm not a maintainer here, but I suggest proving that this code is actually better than before (i.e. disable the cache and run some benchmarks) |
|
The existing code is buggy and hard to read. There are performance gains but those are secondary to making the code maintainable, which is the real intent here. Benchmarking this in a meaningful way is tricky because while on average the performance gains are modest (0.5-1ms per url) those gains across an entire package tree add up, and there is the unmeasured win of no longer being susceptible to regular expression denial of service attacks |
|
There's no changelog entries - what are the breaking changes here? |
|
the breaking change was in how external providers are declared, the changes to |
|
Thanks. could the changelog get entries added for v4+? |
this module was pretty slow based on the usage of regular expressions, unnecessary loops, and even a custom string templating engine.
more importantly than that, it was also difficult to read, difficult to add tests, and difficult to debug.
this rewrite eliminates all of that.
based on #79 which was the test-only refactor, definitely review and land that first