Add dev warning when type prop is missed for checkbox, radio or multi…

[Andarist]

Simple PR improving DX by providing a helpful warning to the developer about his/her mistake.

[erikras]

The warning function already does the env check. You can remove that.

[erikras]

Can you add a test? Warnings are tested like this.

[codecov]

Codecov Report

Merging #204 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #204   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         173    181    +8     
  Branches       58     63    +5     
=====================================
+ Hits          173    181    +8

Impacted Files	Coverage Δ
src/Field.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd73fac...511216b. Read the comment docs.

[erikras]

This is a great PR. I wonder if this warning shouldn't be in getValue.js since that's where the issue it's addressing is?

[Andarist]

@erikras I was just commenting about env check, here it goes:

Each warning call should be wrapped with explicit NODE_ENV check for better minification, uglifyJS and other minifiers are not always able to dead code eliminate things, you can inspect your minified umd bundle and you will see that log messages are still there

let's consider even really simple example of

// input.js
const warning =
  'production' === 'production'
    ? (condition, message) => {}
    : (condition, message) => {
        if (!condition) {
          console.error(`Warning: ${message}`)
        }
      }

warning(false, 'Log')

// uglifyjs input.js -c --toplevel
((condition,message)=>{})(!1,"Log");

I've also explained this lately in those 2 comments (here && here) if you want to read slightly more.

My recommendation is to wrap each call site to remove all this dead code. I can do it as part of this PR or I can remove this "explicit" env check from this PR and prepare a follow up PR wrapping each call site.

Can you add a test? Warnings are tested like this.

Gonna look into it in a moment and ping u when I'm done with it.

I wonder if this warning shouldn't be in getValue.js since that's where the issue it's addressing is?

I've briefly thought about it, but getValue doesn't have access to props and it was easier to check this and construct more helpful message here. Also this problem affects render first but we cannot warn there because we don't know what user will render and to what kind of DOM element it will hook the handlers.

This is a great PR.

Thanks! I thought you are gonna like it 😉

[erikras]

Each warning call should be wrapped with explicit NODE_ENV check for better minification

Awesome, I love being proved wrong! Makes total sense. I definitely wouldn't mind a PR converting all the other usages of warning...hint...hint... 🤔 😃

[erikras]

...actually, now that I think about it, this revelation kind of renders the warning function totally useless. 😆

[SimenB]

This is great! We were unable to get radios working properly with a custom component before sending in type to Field. Awesome with a great warning 🙂

[Andarist]

aint sure if this is entirely correct, if not - let me know what should be changed about it

[Andarist]

I think those are exclusive things, right?

[erikras]

Hopefully no one wants a <select type="radio">. 🤣

[Andarist]

I find snapshots really nice for those kind of tests ("warning" text/errors thrown snapshots), but you do not use any snapshots so I wasn't sure if you'd like this approach.

[erikras]

I only just recently understood snapshot testing. Not actively against it, but not excited to migrate any tests that could be more concise with snapshots... 🤷‍♂️

[Andarist]

@erikras provided a test, please take a look :)

Awesome, I love being proved wrong! Makes total sense. I definitely wouldn't mind a PR converting all the other usages of warning...hint...hint... 🤔 😃

Sure thing, gonna send additional PR in a minute.

...actually, now that I think about it, this revelation kind of renders the warning function totally useless.

It still encapsulates branching logic, not much - but might be nice to keep it. We'll discuss this under upcoming PR.

[erikras]

Published in v3.2.1.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.