search

paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 389 forks source link

Combine String#trim regex into one #284

Closed arlolra closed 10 years ago

ljharb commented 10 years ago

String replacement with the $ can be problematic in IE - can you confirm that the relevant tests pass in IE 6, 7, and 8 particularly?

arlolra commented 10 years ago

Not easily, no. Shouldn't the CI setup be testing in the legacy engines you want to support?

ljharb commented 10 years ago

Yes, ideally, but the current test setup doesn't make it easy to set this up across all supported browsers. Is there a reason you made this change in the first place? (ie, a browser/engine in which this performs better, or fixes a bug)

I may be able to test this, but in the meantime I'll leave this open and unmerged.

arlolra commented 10 years ago

I upgraded from v0.10.0 to v0.16.0 and noticed a performance regression. Bisecting led me to 209a92daa081467ea50584a258e1215e4afae32f. The loss of the native trim.

Staring at the code, the changes seemed like an improvement (doing less work) but that statement isn't evidence based.

Maybe try, https://saucelabs.com/javascript/mocha-js

ljharb commented 10 years ago

I've found a way to combine the regexen into one without using "$" replacement. I'll commit that in which should close this issue.

As for performance - I'm very open to ideas to increase performance, accompanied by profiling numbers - please feel free to open a separate issue/PR for that.