Audit @flow and "use strict" #3451

mpontus · 2017-05-03T13:02:15Z

These changes address the proposal from #3169

Summary

"use strict" was removed and replaced with transform-strict-mode babel plugin.
Contributing guidelines and dangerfile have been updated to reflect this change.
@flow annotations have been added to some files

In regards to the use of "use strict" there remains to be an issue with following file:
https://github.com/facebook/jest/blob/master/packages/jest-util/src/setGlobal.js

Babel plugin is going to add "use strict" annotation to the compiled file which will break unit tests for Node 7.3.

Summary of all failing tests
 FAIL  packages/jest-runtime/src/__tests__/Runtime-cli-test.js (6.138s)
  ● Runtime › cli › always disables automocking

    expect(received).toMatch(expected)
    
    Expected value to match:
      "Hello, world!
    "
    Received:
      ""
      
      at Object.it (packages/jest-runtime/src/__tests__/Runtime-cli-test.js:46:29)
      at process._tickCallback (internal/process/next_tick.js:103:7)

 FAIL  integration_tests/__tests__/env-test.js (5.488s)
  ● Environment override › uses jsdom when specified

    TypeError: Cannot read property 'trim' of undefined
      
      at getLog (integration_tests/__tests__/env-test.js:13:65)
      at Object.it (integration_tests/__tests__/env-test.js:19:12)
      at process._tickCallback (internal/process/next_tick.js:103:7)

Following files were left without @flow annotations. As far as I know typechecking them would require some changes to the codebase which I was hesitant to introduce without explicit instructions.

https://github.com/facebook/jest/blob/master/packages/jest-config/src/vendor/jsonlint.js
https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/jasmine/createSpy.js
https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/jasmine/Env.js

Test plan
Verify that typechecking produces no errors:

$ yarn test
yarn run v0.23.2
$ yarn run typecheck --silent && yarn run lint --silent && yarn run jest --silent && yarn run test-examples --silent 
....
Ran all test suites in 10 projects.
Done in 81.75s.

Find files processible by babel with "use strict" (should be no files)

$ find packages/*/src -name '*.js' -not \( -path '*/__tests__/*' -or -path '*/__mocks__/*' \) | xargs -d "\n" grep -l "'use strict'"

Find package source files without @flow

$ find packages/*/src \( -name '*.js' -or -name '*.jsx' \) -not \( -path '*/__tests__/*' -or -path '*/__mocks__/*' \) |xargs -d "\n" grep -L '@flow'
packages/jest-config/src/vendor/jsonlint.js
packages/jest-jasmine2/src/jasmine/createSpy.js
packages/jest-jasmine2/src/jasmine/Env.js

mpontus · 2017-05-03T20:32:46Z

I reverted the removal of "use strict" from test and mock files. These files will not be processed by babel during the execution of integration tests, and will cause them to fail for Node 4 which does not permit the use of block-scoped declarations (let) outside of strict mode.

codecov-io · 2017-05-03T23:03:13Z

Codecov Report

Merging #3451 into master will increase coverage by 0.11%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #3451      +/-   ##
==========================================
+ Coverage   62.28%   62.39%   +0.11%     
==========================================
  Files         181      181              
  Lines        6714     6646      -68     
  Branches        6        6              
==========================================
- Hits         4182     4147      -35     
+ Misses       2529     2496      -33     
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-cli/src/lib/updateArgv.js	`95.23% <ø> (ø)`	⬆️
packages/jest-validate/src/errors.js	`100% <ø> (ø)`	⬆️
packages/jest-validate/src/index.js	`0% <ø> (ø)`	⬆️
packages/jest-util/src/formatTestResults.js	`0% <ø> (ø)`	⬆️
packages/jest-jasmine2/src/p-timeout.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/lib/getTestPathPattern.js	`60% <ø> (ø)`	⬆️
...ckages/jest-config/src/reporterValidationErrors.js	`20% <ø> (-13.34%)`	⬇️
packages/jest-test-typescript-parser/src/index.js	`0% <ø> (ø)`	⬆️
packages/jest-haste-map/src/worker.js	`94.59% <ø> (ø)`	⬆️
packages/jest-cli/src/generateEmptyCoverage.js	`90% <ø> (ø)`	⬆️
... and 183 more

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 6a12116...2eb2769. Read the comment docs.

thymikee

Thanks, this looks good! I think we could further improve types in Jasmine, but maybe not in this diff. Also @wtgtybhertgeghgtwtg is probably working on that already.

thymikee · 2017-05-04T06:33:16Z

packages/jest-jasmine2/src/jasmine/ReportDispatcher.js


-function ReportDispatcher(methods) {
+function ReportDispatcher(methods: Object) {


methods are an array, could you improve this type?

thymikee · 2017-05-04T06:36:21Z

packages/jest-matchers/src/jasmine-utils.js

  var result = true;

  var asymmetricResult = asymmetricMatch(a, b);
-  if (!isUndefined(asymmetricResult)) {
+  if (asymmetricResult !== void 0) {


undefined maybe?

mpontus · 2017-05-04T18:30:39Z

Thanks @thymikee! I updated my pull request as per your suggestions.

On a related note, would it be acceptable if I rebase my branch on top of jest/master while pull request is open?

thymikee · 2017-05-04T18:32:05Z

Of course it is acceptable! :)

thymikee

Just this one change and I can approve this :)

thymikee · 2017-05-05T12:49:08Z

package.json

@@ -14,6 +14,7 @@
    "babel-preset-env": "^1.4.0",
    "babel-preset-react": "^6.24.1",
    "babel-preset-react-native": "^1.9.1",
+    "babel-plugin-transform-strict-mode": "^6.22.0",


Can we use version 6.24.1? We already have that in our transitive deps (see yarn.lock)

jest-bot · 2017-05-05T18:22:29Z

	Warnings
⚠️	Changes were made to package.json, but not to yarn.lock - Perhaps you need to run `yarn install`?

Generated by 🚫 dangerJS

* 'use strict' removed from pacakges' sources * Strict mode is now enabled automatically by babel * 'use strict' check removed from dangerfile * Updated contributing guidelines

cpojer · 2017-05-12T10:02:01Z

The node 7.3 thing, I don't care any more. That was reverted and we don't need it. We can tell people to either downgrade Jest or upgrade node if it happens.

* Remove 'use strict' from all files * 'use strict' removed from pacakges' sources * Strict mode is now enabled automatically by babel * 'use strict' check removed from dangerfile * Updated contributing guidelines * Enable @flow for most files * Commit yarn.lock * Revert removal of "use strict" in test and mock files * Improve annotations for ReportDispatcher * Replace "void 0" with "undefined" * Update babel-plugin-transform-strict-mode to match transitive deps

github-actions · 2021-05-13T15:07:25Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label May 3, 2017

mpontus mentioned this pull request May 4, 2017

Audit flow + "use strict" #3169

Closed

thymikee reviewed May 4, 2017

View reviewed changes

mpontus force-pushed the master branch from 140378c to e9f5009 Compare May 5, 2017 04:40

thymikee requested changes May 5, 2017

View reviewed changes

thymikee approved these changes May 5, 2017

View reviewed changes

mpontus added 7 commits May 12, 2017 10:46

Remove 'use strict' from all files

8fcf86e

* 'use strict' removed from pacakges' sources * Strict mode is now enabled automatically by babel * 'use strict' check removed from dangerfile * Updated contributing guidelines

Enable @flow for most files

d551afc

Commit yarn.lock

0422ce4

Revert removal of "use strict" in test and mock files

353a06c

Improve annotations for ReportDispatcher

aa72328

Replace "void 0" with "undefined"

4595f56

Update babel-plugin-transform-strict-mode to match transitive deps

2eb2769

cpojer force-pushed the master branch from ea66cac to 2eb2769 Compare May 12, 2017 09:56

cpojer merged commit 9b157c3 into jestjs:master May 12, 2017

renovate-bot mentioned this pull request May 17, 2017

Update dependency jest to version 20.0.4 renovatebot/renovate#208

Merged

renovate-bot mentioned this pull request May 17, 2017

Update dependency babel-jest to version 20.0.3 renovatebot/renovate#207

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit @flow and "use strict" #3451

Audit @flow and "use strict" #3451

mpontus commented May 3, 2017 •

edited

Loading

mpontus commented May 3, 2017

codecov-io commented May 3, 2017 •

edited

Loading

thymikee left a comment

thymikee May 4, 2017

thymikee May 4, 2017

mpontus commented May 4, 2017

thymikee commented May 4, 2017

thymikee left a comment

thymikee May 5, 2017

jest-bot commented May 5, 2017 •

edited

Loading

cpojer commented May 12, 2017

github-actions bot commented May 13, 2021


		function ReportDispatcher(methods) {
		function ReportDispatcher(methods: Object) {

Audit @flow and "use strict" #3451

Audit @flow and "use strict" #3451

Conversation

mpontus commented May 3, 2017 • edited Loading

mpontus commented May 3, 2017

codecov-io commented May 3, 2017 • edited Loading

Codecov Report

thymikee left a comment

Choose a reason for hiding this comment

thymikee May 4, 2017

Choose a reason for hiding this comment

thymikee May 4, 2017

Choose a reason for hiding this comment

mpontus commented May 4, 2017

thymikee commented May 4, 2017

thymikee left a comment

Choose a reason for hiding this comment

thymikee May 5, 2017

Choose a reason for hiding this comment

jest-bot commented May 5, 2017 • edited Loading

cpojer commented May 12, 2017

github-actions bot commented May 13, 2021

mpontus commented May 3, 2017 •

edited

Loading

codecov-io commented May 3, 2017 •

edited

Loading

jest-bot commented May 5, 2017 •

edited

Loading