UPGRADE react & friends && UPGRADE other dependencies #1908
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1908 +/- ##
==========================================
+ Coverage 21.29% 21.43% +0.13%
==========================================
Files 261 261
Lines 5743 5743
Branches 687 687
==========================================
+ Hits 1223 1231 +8
+ Misses 4002 3994 -8
Partials 518 518
Continue to review full report at Codecov.
|
addons/knobs/src/KnobManager.test.js
Outdated
| name: 'foo', | ||
| }), | ||
| }; | ||
| testManager.knobStore = { set: jest.fn(), get: () => ({ defaultValue: 'default value', value: 'current value', name: 'foo' }) }; |
There was a problem hiding this comment.
@ndelangen what's going on here? any chance we fix the linting setup so that our code is not filled with non-functional changes like this again and again? i think this is probably the fifth PR that i've looked at over the past few months that contains hundreds of lines of changes that have nothing to do with the functionality of the code. it looks really pretty when you are generating visualizations of the codebase, but wastes a lot of time for PR review and i've heard from others who are also annoyed about this. curious to hear your thoughts.
| type: 'text', | ||
| }, | ||
| }; | ||
| const knobs = { foo: { name: 'foo', value: 'default string', type: 'text' }, baz: { name: 'baz', value: 'another knob value', type: 'text' } }; |
There was a problem hiding this comment.
@ndelangen related to my comment above, i'm surprised that the linter allows long lines like this?
|
@ndelangen are there meaningful changes in here that make it work with react 16+? what happens when you run the new code with a react 15 project? what happens when you run the old code with a react 16 project? understanding that would help me evaluate the PR. it's currently hard to evaluate because most of the changes are non-functional and unrelated to the react 16 upgrade. |
|
I can see quite a lot of lint warnings on CI, looks like @ndelangen had an outdated prettier version or something. I'll run |
|
Sorry for the confusion, I'll see where this outdated prettier is coming from.. thanks @Hypnosphi ! 👍 |
|
@shilman @ndelangen How do you guys feel about raising prettier from |
|
I feel inconsequential whitespace issues should not fail a build. That's just not productive. The point is to make it easier on everyone. |
|
The only way those errors could appear is that someone had explicitly turned off git hooks, which is not good by itself. Prettier errors are 100% autofixable. And I agree with @shilman that those irrelevant diffs are quite annoying for reviewers |
|
BTW @igor-dv had some suggestion and a possible fix for the unit test I had trouble with. |
|
@ndelangen There is still some unneeded diff left, which can only be reverted manually because prettier is ok with both "before" and "after" states in those cases |
|
I'm totally agree with @shilman. These changes will also be a pain in the ass when solving conflicts in the release branche |
|
I'll remove the unrelated changes ASAP. |
ndelangen
force-pushed
the
ndelangen/upgrade-react
branch
from
9890135 to
02c6198
Compare
|
The test changes look OK to me |
docs/package.json
Outdated
| "react": "^15.6.1", | ||
| "react-document-title": "^2.0.3", | ||
| "react-dom": "^15.6.1", | ||
| "react-dom": "^15.5.0", |
app/vue/package.json
Outdated
| "core-js": "^2.5.1", | ||
| "css-loader": "^0.28.7", |
There was a problem hiding this comment.
Please remove the duplication (see lint errors)
|
I wonder why there are no changes in lockfiles |
|
@Hypnosphi can you review / approve? Anyone else on the @storybooks/team wants to review this? |
Issue: -upgrade to react 16.0.0-
What I did
How to test
Run all tests and examples
Notes
I had difficulty testing
/lib/ui/src/modules/ui/components/left_panel/stories_tree/index.test.jsI narrowed down the test, would love some help getting this tested properly!