search

dougwilson / nodejs-depd

Deprecate all the things
MIT License
325 stars 87 forks source link

Change eval to Immediately-invoked function #22

Closed gseok closed 6 years ago

gseok commented 7 years ago

javascript 'eval' function is bad practice. and 'eval' is prohibited in CSP(https://developer.chrome.com/apps/contentSecurityPolicy).

And... In many lint programs it is detected as a bad habit. so .. I think it should be fixed.

gseok commented 7 years ago

I understood your intentions.

You are creating a wrapper function with the same number of function param(arguments) as the original function's param(arguments). and wrapper function added log function call in the form of a closure.

I'm try to wrapper function call log and original function call with don't using 'eval' or 'new Function'. But... there is no way. I think.. your 'eval' code is only way...But.! as you know.. 'eval' is not good...

Theoretically, the number of params in a function can be infinite. However, when you create an actual function, you usually create 10 to 20 parameters. In fact, a function that uses more parameters is a function of poor design. Or functions that need to separate functions(refactoring).

'Eval' does not use 'new Function', and there seems to be no way to behave as intended. What if you do not use 'eval' or 'new Function', but actually cover only functions with about 10 to 20 params?

sudo-suhas commented 6 years ago

Hey @dougwilson, I see that you have a test for the function arity: https://github.com/dougwilson/nodejs-depd/blob/99777ae9403325b7e364d5e5d74d1226e7b9acc1/test/test.js#L293-L295

Could you please tell me why this is important?

dougwilson commented 6 years ago

Hi @sudo-suhas the test suite covers the features the library provides. The helper to automatically wrap functions leaves the arity intact becuase there are various libraries all over npm that use fn.length to make decisions, typically when they changed their interface. Express.js also uses fn.length to determine the type of middleware a function is. I hope that helps.

dougwilson commented 6 years ago

Also looks like coverage is no longer 100% if you can add more tests for all new code branches to get back to 100%.