New API change: Delegate isEmpty() to exists() and begin deprecation of using isEmpty() #722
Conversation
|
I've also fixed the gitbook prism plugins version number to the latest that works. When I tried running the docs it kept failing, which I narrowed down to the latest release of the prism plugin. |
|
(would be nice to rebase down to a single commit) |
christemple
force-pushed
the
change_isempty_to_exists_and_alias
branch
from
fc3c98d
to
56a62fe
Compare
|
@ljharb done 👍 |
| * | ||
| * @returns {boolean} | ||
| */ | ||
| isEmpty() { | ||
| return this.length === 0; | ||
| // eslint-disable-next-line no-console | ||
| console.warn('Deprecated method isEmpty() called, use exists() instead.'); |
There was a problem hiding this comment.
Is it worth ensuring we only log this once? I imagine anyone using isEmpty getting 100s of these warnings..
There was a problem hiding this comment.
I think it would be nice to prefix this with Enzyme:: just so people know where the warning is coming from.
There was a problem hiding this comment.
Yeah sounds like a good idea, I'll update this tomorrow, thanks
There was a problem hiding this comment.
Actually I'm wondering if we should hold off on the deprecation notice in-code for now, until we've been able to make PRs into things like chai-enzyme and others - ie, give a whole release before adding the warning message in?
There was a problem hiding this comment.
@ljharb up to you. But I always just put deprecation warnings in on minor versions and pull them out in next major versions. I think react adds deprecations in majors only?
There was a problem hiding this comment.
@blainekasten I definitely think prefixing with Enzyme:: makes sense, however after thinking more about it, I'm not so sure about logging it only once.
If someone uses isEmpty() a lot isn't it better to make noise to encourage people to make the switch sooner rather than later so the tech debts lifespan is short?
The noise was very helpful for me, after switching off mocha --reporter dot, to identify and locate specs using isEmpty so that I could update them, as isEmpty() is the equivalent of !exists() it wasn't a matter of doing a global search and replace on the specs.
There was a problem hiding this comment.
I'm not sure how we'd "only log once" - store a global? what's "once" - if i'm using --watch with my tests, should only the first run log it?
I think if it logs, it has to log every time.
There was a problem hiding this comment.
I'm not sure how we'd "only log once" - store a global?
React uses a scoped flag to only log warnings once, but I think in our case it makes the most sense to just log every time. Test output can be rather verbose, making a single warning potentially hard to distinguish. isEmpty isn't likely used with extreme frequency in the average test suite either, so in most cases we're probably not going to be outputting too much noise.
Also, in regards to versioning: semver specifies that deprecation warnings should go in minor releases so I'm 👍 with that.
There was a problem hiding this comment.
Good point @ljharb. --watch mode defeats the ability to store a global. Logging every time seems like the best option for us. Good discussion guys!
There was a problem hiding this comment.
I've just done the update to prefix Enzyme:: in the warning
christemple
force-pushed
the
change_isempty_to_exists_and_alias
branch
from
56a62fe
to
98d7961
Compare
|
lgtm! Thanks |
To address the issue whereby
isEmpty()being confusing from an API perspective when checking if a nodeexists().#705
Includes:
exists()function and delegatingisEmpty()toexists()isEmpty()exists()and deprecation docs forisEmpty()