[chore] Fix a number of alerts found on LGTM.com #287
Conversation
needs to be checkeg against "number" not "Number"
jest.runCLI() was being called with only 1 argument, there was a mis-bracketing which lead to this. When trying to fix the bracketing here, I realised the error handling callback also did not work correctly (done was undefined), and the 'test' task considered itself succeeded before the tests were even run. I've refactored the task a little to successfully "fail" when the tests fail, which means that "gulp test" will now return a non-zero exit code when the tests fail.
React component state updates using setState may asynchronously update this.props and this.state, thus it is not safe to use either of the two when calculating the new state passed to setState, and instead the callback-based variant should be used instead. There is one more instance remaining, where the a state property is set to the value of itself, which I thought should be discussed before fixing.
|
There was one alert for inconsistent state which I did not change, as the code looks peculiar to me and wanted to check first what it was doing: react-responsive-carousel/src/components/Thumbs.js Lines 221 to 225 in 9ded7e8 The effect of line 244 should be nothing, unless you're relying on unreliable implementation details of react and dispatching of state updates. I've left this as is for now, but I think it should probably be removed? |
|
Thanks for your contributions @samlanning, I'll publish this ASAP |
Yes, that block of code doesn't make sense! Happy to accept a PR fixing that :) Thanks |
This isn't a normal feature/bugfix PR, and I'm unsure if this is the sort of contribution you'd like to receive, but thought there's no harm in trying :)
This PR fixes a number of alerts that I found on LGTM.com, and there are a bunch of other alerts still remaining for this project that would probably also be good to fix.
I also reworked the gulp test task a little more in f7e5810 (details in commit message)
(Full disclosure: I'm part of the team behind LGTM.com)