feat: replace Map by plain old objects to support more browsers #78
Conversation
Since we use basic Map features (get, set, has, delete) only, we can as well just replace our maps by objects to support IE < 11 and other browser that do not recognize ES6 specifications. This keeps the bundle small and avoids the need of costy polyfills.
…t supported" This reverts commit 13b1c0e.
src/perfume.ts
Outdated
| @@ -198,7 +206,7 @@ export default class Perfume { | ||
| // Get duration and change it to a two decimal value | ||
| const duration = this.perf.measure(metricName, metric); | ||
| const duration2Decimal = parseFloat(duration.toFixed(2)); | ||
| this.metrics.delete(metricName); | ||
| delete this.metrics.metricName; |
There was a problem hiding this comment.
This one should probably be delete this.metrics[metricName]
There was a problem hiding this comment.
both is valid syntax. see https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Operators/delete
There was a problem hiding this comment.
This example should help explain my point
which is, that delete this.metrics.metricName will delete the Key value metricName, and not the string contained inside the metricName variable.
Also, I should probably add a test to prevent this issue.
There was a problem hiding this comment.
Oh, I see your point. Will fix that...
There was a problem hiding this comment.
have fixed it. added a test to check proper deletion of metrics. this test fails with the previous implementation and passes with the fix as expected. :)
src/perfume.ts
Outdated
| @@ -164,14 +172,14 @@ export default class Perfume { | ||
| if (!this.checkMetricName(metricName)) { | ||
| return; | ||
| } | ||
| if (this.metrics.has(metricName)) { | ||
| if (this.metrics.hasOwnProperty(metricName)) { |
There was a problem hiding this comment.
Based on https://jsperf.com/map-has-vs-object-hasownproperty results,
the most performant way would be simply to
if (this.metrics[metricName]) {There was a problem hiding this comment.
good catch. Going to update this to use the fastest :) thx
|
The code has been published under v3.0.0-beta.0 Please let me know if works all good! Also, v3 is going to have some nice extra feature, but before the full roll-out, I'm going to have a couple of extra betas. |
|
thanks, have installed the new beta and will test it in the wild ;) |
Since we use basic Map features (get, set, has, delete) only, we can
as well just replace our maps by objects to support IE < 11 and other
browser that do not recognize ES6 specifications.
This keeps the bundle small and avoids the need of costly polyfills. We, the performance measurement teams, have a special interest in keeping our performance impact as low as possible.