audiojs / contributing

Discussion and guidelines for contributing
2 stars 0 forks source link

Using `assert` + `unassertify` #39

Closed jamen closed 6 years ago

jamen commented 7 years ago

I've seen a neat pattern in modules to use asserts to verify types and do more fine debugging:

function something (options) {
  assert.equal(typeof options, 'object', 'options must be an object')
  // ...
}

This way you get friendly errors in development, then you use something like unassertify in production to strip all the validation away and get smaller bundles.

One downside is that we do have to tell users to remove the asserts or else their bundle sizes will be a bit larger... I do not think this is too huge of a deal.

dy commented 7 years ago

Yes, looks like a nice pattern, the approach of choo. Although assert is not elaborated much, eg. sometimes need to do something like

if (!(a instanceof ArrayBuffer)) throw Error('arg should be an array buffer')

It really depends on component and use case I think, it is easy to fall into covering imaginary wrong arguments instead of minimal checks. But in general I am for.

dy commented 7 years ago

@jamen I use assertions here, feels good :) Do I do it right?

jamen commented 7 years ago

@dfcreative Yup looks good to me. :smile_cat:

vectrixdevelops commented 7 years ago

I will also apply this to speaker to replace the debug module. I'll cook it in with the many other formatting fixes I want to make.

dy commented 7 years ago

That seems to be good option for browser, but is there a good way to disable assertions for node?

jamen commented 6 years ago

@dfcreative Hmm that is a great point. Is there something bad about having the asserts in production with node?

dy commented 6 years ago

Recently I had short discussion on that, basically we can detect ifs with single throw statement statically, so there is no much need in unassertify at all.

jamen commented 6 years ago

Okay, I like the sound of that better, so I was checking in. assert was not as good in retrospect.