Upgrade to babel@7 #1043

Andarist · 2018-10-10T11:31:21Z

Obviously I can't be 100% sure if this covers everything - I'd appreciate review on that. I've check build + tests and they seems to work fine.

I'd also like to get your opinion on 1 thing as it stays unfinished. Currently each test for eact versions runs ALL versions instead of a specific one. We can fix it in 2 ways:

make each test script run only test files in specific directory
run tests from all directories in a single jest process

Which one do you prefer? (I would argue latter is better, simpler & coverage might be collected to a single file)

netlify · 2018-10-10T11:32:39Z

Deploy preview for react-redux-docs ready!

Built with commit 65af5d9

https://deploy-preview-1043--react-redux-docs.netlify.com

markerikson · 2018-10-12T18:31:04Z

@cellog , any thoughts on the test setup stuff?

cellog · 2018-10-13T00:11:41Z

Sorry I've been awol, things are still going to keep me away for a couple more weeks. If there is a specific thing I can look at quickly, feel free to ping me on Discord and I'll comment here

Andarist · 2018-10-13T19:37:49Z

@cellog you can skip the code changes and just read the description of this PR

cellog · 2018-10-15T02:33:40Z

The point is to run tests against all versions of React supported by react-redux. So this should remain unchanged. Coverage is generated for all, and codecov just picks up one of them automatically. Since we don't have code that runs differently in different versions, it doesn't really matter. But running against all versions does. 5.1.0-test-1 broke prior to this because it wasn't possible to test against other versions.

Thus, you can't run them in a single jest process, as it would require unloading/reloading react (which isn't possible)

cellog · 2018-10-15T02:34:25Z

P.S. thank you for this contribution! (should have said that first). Looks great, I think (on a cursory visual scan)

Andarist · 2018-10-15T07:31:43Z

Thus, you can't run them in a single jest process, as it would require unloading/reloading react (which isn't possible)

I wasnt talking about restructuring how tests are written now. It's possible (I think) to rung them all in a single process as each test directory (one per react version) has its own react version located in its node_modules, so each test should find correct (local~) react version. I'll give it a try to showcase what I'm talking about.

cellog · 2018-10-15T11:31:14Z

Hi again. Actually, my first attempt to write this did exactly what you suggested. Perhaps jest 26 is different, but with 24, it would always load react from the global package.json. Then, when loading enzyme (which we used at the time) it would error out in weird ways. I couldn't convince jest to load the local package versions of dependencies of the packages in the local package.json (I think: memory is foggy on exact details of the error). In essence it's npm's cute little version of Windows 3.1 DLL hell :).

FYI, it is possible to run only one react version using:

REACT=16.4 npm test

For example. Would that solve the issue you're experiencing?

cellog · 2018-10-15T11:31:46Z

Incidentally, we need to add 16.5 as a test version, if anyone is up to it

Andarist · 2018-10-15T22:15:51Z

I've tweaked the PR - it's still possible to run tests against a single version of react, but tests for all versions are ran within single jest process (coverage is collected only from a single version) - this might be slightly faster as we dont have to spawn X processes, one after another. The bigger gain is that watch mode might be used slightly more efficiently. I understand that we copy both source and test files to version directories, so it's not ideal as changes in true sources & tests wont be reflected, but its still better than before as we can edit files contained in any directory within a single watch process.

I've confirmed that tests are not ran against a single react version - each directory requires its own version of react.

Additionally I've added tests against react@16.5 as requested.

Andarist · 2018-10-15T22:17:27Z

test/react/16.5/test/getTestDeps.js

@@ -0,0 +1,7 @@
+import enzyme from 'enzyme'


this seems like leftover from refactoring fro menzyme to react-testing-library, should I remove all those files (they are in each, or most, directories)?

Yes, references to them were removed in #998.

Oh, looks like that removed the react/${version}/test/ directory, which is needed. Can you add empty .keep files back to keep the dirs in git, or add something to the install script to create them?

Andarist · 2018-10-15T22:19:50Z

test/run-tests.js

+    // every directory has the same coverage, so we collect it only from one
+    collectCoverageFrom: [`react/${LATEST_VERSION}/src/**.js`]
+  }
+  npmRun.execSync(`node ./node_modules/.bin/jest -c '${JSON.stringify(allVersionsConfig)}'`, { stdio: 'inherit' })


it seems to me that node ./node_modules/.bin/jest can be used when testing a specific version too, combining this with rootDir option would allow us to remove usage of npm-run here, removing cd dance & removing 'artificial' test scripts from nested package.jsons

WDYT?

First, if we're using npm-run, then this should just be jest -c .... That module automatically adds the .bin dir to the PATH.

We could actually make use of jest --projects in this case. That might be easier, as each version of React would be testable in isolation and without as much tooling.

First, if we're using npm-run, then this should just be jest -c .... That module automatically adds the .bin dir to the PATH.

Cool, didnt know that - gonna chang it in a minute.

We could actually make use of jest --projects in this case. That might be easier, as each version of React would be testable in isolation and without as much tooling.

I thought about it, but I think that would require keeping a copy of the jest config per each react version and is IMHO annoying in a long run (when it needs updating).

Also I wasnt sure how to collect coverage in --projects case as we wont to collect it only from a single project.

Damn, it doesn't do per-project coverage? That seems like an oversight.

I guess it might have, we just have exactly the same coverage here per project as noticed by @cellog. I guess overcollecting the coverage doesnt do us harm, but im not sure how CI takes the coverage for its report. Is the path configurable? Gonna look into that later as im on the mobile right now. If its configurable then i guess we might just point to the latest react directory and be done with it (leting jest collect in each directory).

There is still an issue of having to duplicate jest config all over the place though (if u want to use the projects feature)

timdorr

LGTM.

Andarist · 2018-10-24T18:04:30Z

@markerikson any thoughts? :)

timdorr · 2018-10-24T18:53:45Z

I'm pretty content with just merging this in. The conflicts for #995 and #1000 should be relatively minimal and easily resolved.

So, I'll do that now :)

Andarist · 2018-10-24T19:05:15Z

Cool, thanks!

* Upgrade to babel@7 * Run tests for all versions of React in a single jest process * Add tests against react@16.5 * Simplify test script * Remove unused getTestDeps.js files & adjusted CONTRIBUTING.md info * Create test directories if they are missing

Upgrade to babel@7

88a8324

itskumaradarsh approved these changes Oct 12, 2018

View reviewed changes

Andarist added 2 commits October 16, 2018 00:02

Run tests for all versions of React in a single jest process

25679b6

Add tests against react@16.5

a8689be

Andarist commented Oct 15, 2018

View reviewed changes

Andarist added 3 commits October 16, 2018 16:40

Simplify test script

cdbc49e

Remove unused getTestDeps.js files & adjusted CONTRIBUTING.md info

83861cb

Create test directories if they are missing

65af5d9

timdorr approved these changes Oct 17, 2018

View reviewed changes

timdorr merged commit 2093fbb into reduxjs:master Oct 24, 2018

Andarist deleted the babel7 branch October 24, 2018 19:04

timdorr mentioned this pull request Oct 25, 2018

(feat): support react 16.6 #1062

Merged

snyk-bot mentioned this pull request Aug 14, 2019

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Watemlifts/wagtail#48

Open

This was referenced Aug 30, 2019

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 iliutastoica/react-slingshot#3

Open

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 jakeherp/react-slack#6

Merged

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 iliutastoica/robofriends-pwa#4

Open

This was referenced Sep 22, 2019

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Matthelonianxl/lisk-nano#9

Open

[Snyk] Upgrade react-redux from 5.0.6 to 5.1.1 Matthelonianxl/lisk-hub#9

Open

[Snyk] Upgrade react-redux from 5.0.7 to 5.1.1 maxiplux/ReduxSimpleStarter#3

Merged

dependabot bot mentioned this pull request Mar 18, 2021

chore(deps): bump react-redux from 4.4.10 to 5.1.2 hkosova/swagger-ui#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to babel@7 #1043

Upgrade to babel@7 #1043

Andarist commented Oct 10, 2018 •

edited

netlify bot commented Oct 10, 2018 •

edited

markerikson commented Oct 12, 2018

cellog commented Oct 13, 2018

Andarist commented Oct 13, 2018

cellog commented Oct 15, 2018

cellog commented Oct 15, 2018

Andarist commented Oct 15, 2018

cellog commented Oct 15, 2018

cellog commented Oct 15, 2018

Andarist commented Oct 15, 2018 •

edited

Andarist Oct 15, 2018

timdorr Oct 16, 2018

timdorr Oct 17, 2018

Andarist Oct 17, 2018

Andarist Oct 15, 2018

timdorr Oct 16, 2018

Andarist Oct 16, 2018

timdorr Oct 16, 2018

Andarist Oct 16, 2018

timdorr left a comment

Andarist commented Oct 24, 2018

timdorr commented Oct 24, 2018

Andarist commented Oct 24, 2018

Upgrade to babel@7 #1043

Upgrade to babel@7 #1043

Conversation

Andarist commented Oct 10, 2018 • edited

netlify bot commented Oct 10, 2018 • edited

markerikson commented Oct 12, 2018

cellog commented Oct 13, 2018

Andarist commented Oct 13, 2018

cellog commented Oct 15, 2018

cellog commented Oct 15, 2018

Andarist commented Oct 15, 2018

cellog commented Oct 15, 2018

cellog commented Oct 15, 2018

Andarist commented Oct 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdorr left a comment

Choose a reason for hiding this comment

Andarist commented Oct 24, 2018

timdorr commented Oct 24, 2018

Andarist commented Oct 24, 2018

Andarist commented Oct 10, 2018 •

edited

netlify bot commented Oct 10, 2018 •

edited

Andarist commented Oct 15, 2018 •

edited