-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix "return" outside of function issue #5
Conversation
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.
IIFEs are obsolete; that's never an acceptable solution.
I very explicitly chose to export the proper function, which will be much faster, rather than polluting a single function with all the logic of each case.
If I decide to change my perfectly valid node CJS module to avoid top-level return, I'll use another approach.
If you don't want to introduce changes due to some philosophical principle that's another thing, but this change is perfectly viable. |
There's a cost to all those if statements if |
Gotcha, I'll try to run some benchmarks later to measure the impact on performance. |
Closes #5. Closes #4. Closes #3. Closes inspect-js/node-deep-equal#78. Closes es-shims/Promise.allSettled#7. Closes airbnb/js-shims#12. Relates to inspect-js/is-set#4, inspect-js/is-set#5 Addresses storybookjs/storybook#9154.
Ran some basic benchmark with nanoseconds accuracy with this snippet: const ITERATIONS = BigInt(100000)
const CASES = ['A', 'B']
const bench = (fn) => {
let total = BigInt(0)
for (let i = 0; i < ITERATIONS; i++) {
const start = process.hrtime.bigint()
fn()
const end = process.hrtime.bigint()
total += (end - start)
}
return total / ITERATIONS
}
const testCaseA = () => false
const testCaseB = () => { if (true) return false }
const results = [bench(testCaseA), bench(testCaseB)]
const faster = results[0] < results[1] ? 0 : 1
const slower = 1 - faster
console.log(`Test case A average: ${results[0]}ns`)
console.log(`Test case B average: ${results[1]}ns`)
console.log(`Test case ${CASES[faster]} is faster than test case ${CASES[slower]} by ${results[slower] - results[faster]} nanoseconds on average`) I ran the above 1000 times, here are the results for the first 10 runs (each run ran 100000 iteration of each test case):
In conclusion, the difference is negligible at best (no more than 70 nanoseconds in all runs), and if anything marks the |
Closes inspect-js#5. Closes inspect-js#4. Closes inspect-js#3. Closes inspect-js/node-deep-equal#78. Closes es-shims/Promise.allSettled#7. Closes airbnb/js-shims#12. Relates to inspect-js/is-map#3, inspect-js/is-map#4, inspect-js/is-map#5 Addresses storybookjs/storybook#9154.
Closes inspect-js/node-deep-equal#79. Closes inspect-js/node-deep-equal#78. Closes es-shims/Promise.allSettled#7. Closes airbnb/js-shims#12. Relates to: - inspect-js/is-map#5 - inspect-js/is-map#4 - inspect-js/is-map#3 - inspect-js/is-set#5 - inspect-js/is-set#4 - storybookjs/storybook#9154.
Closes inspect-js/node-deep-equal#79. Closes inspect-js/node-deep-equal#78. Closes es-shims/Promise.allSettled#7. Closes airbnb/js-shims#12. Relates to: - inspect-js/is-map#5 - inspect-js/is-map#4 - inspect-js/is-map#3 - inspect-js/is-set#5 - inspect-js/is-set#4 - storybookjs/storybook#9154.
Should solve these errors - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Bad_return_or_yield
storybookjs/storybook#9154
Corresponding PR in
is-set
- inspect-js/is-set#5