Skip to content

Conversation

skovy
Copy link
Contributor

@skovy skovy commented Jul 13, 2019

Motivation

Now that jscodeshift supports TypeScript (added in v0.6.0) I was exploring upgrading jscodeshift to take advantage of that added support. I was able to get a proof of concept working and planning to open that in a separate PR. However, there were some conflicts between the current babel packages and setup when upgrading jscodeshift and a few cleanup items I was hoping to merge in this PR before opening a PR to propose upgrading jscodeshift and adding TypeScript support (#206).

Changes

Upgrade Dependencies

  • jest: Jest is many major versions behind in this project. Also to take advantage of spyOn it was necessary to upgrade (see below for use case of spyOn).
    • Breaking changes required updating the configuration in package.json to change testPathDirs to roots (Jest CHANGELOG v19.0.0)
  • babel: as recommended in the Jest Getting Started for Using Babel documentation add (or upgrade) the following babel packages:
    • babel-jest (upgraded)
    • @babel/core (peer dependency)
    • @babel/preset-env (this replaces babel-preset-es2015 as documented here)
    • @babel/plugin-proposal-object-rest-spread (this replaces babel-plugin-transform-object-rest-spread)

Update prepublish script

Previously, it was using npm to run lint and test. Given the project is using yarn.lock it seems fair to assume development should be done using yarn? If so, it seems preferable to not mix yarn and npm usage (happy to revert this change if there's opposition as it's not necessary for the purposes of this PR)

Silencing verbose logging during testing

Some of the transforms have custom console.log or console.warn calls to provide information while using the transforms. While running tests (yarn test) these get verbose and make it difficult to see the whole test suite (happy to revert this change if there's opposition as it's not necessary for the purposes of this PR).

[Before] Example of running `yarn test`
yarn run v1.16.0
$ jest
 PASS  transforms/__tests__/rename-unsafe-lifecycles-test.js
 PASS  transforms/__tests__/class-test.js
  ● Console

    console.warn transforms/class.js:325
      /code/react-codemod/transforms/__testfixtures__/class/class.input.js: `MyComponent4` was skipped because of invalid field(s) `foo` on the React component. Remove any right-hand-side expressions that are not simple, like: `componentWillUpdate: createWillUpdate()` or `render: foo ? renderA : renderB`.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-test2.input.js: `ComponentWithInconvertibleMixins` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-test2.input.js: `ComponentWithInconvertibleMixins2` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-pure-mixin3.input.js: `ComponentWithOnlyPureRenderMixin` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:212
      /code/react-codemod/transforms/__testfixtures__/class/class-initial-state.input.js: `PassGetInitialState` was skipped because of API calls that will be removed. Remove calls to `getDefaultProps` and/or `getInitialState` in your React component and re-run this script.
    console.warn transforms/class.js:212
      /code/react-codemod/transforms/__testfixtures__/class/class-initial-state.input.js: `UseGetInitialState` was skipped because of API calls that will be removed. Remove calls to `getDefaultProps` and/or `getInitialState` in your React component and re-run this script.
    console.warn transforms/class.js:228
      /code/react-codemod/transforms/__testfixtures__/class/class-initial-state.input.js: `UseArguments` was skipped because `arguments` was found in your functions. Arrow functions do not expose an `arguments` object; consider changing to use ES6 spread operator and re-run this script.
    console.warn transforms/class.js:293
      /code/react-codemod/transforms/__testfixtures__/class/class-initial-state.input.js: `ShadowingIssue` was skipped because of potential shadowing issues were found in the React component. Rename variable declarations of `props` and/or `context` in your `getInitialState` and re-run this script.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-prune-react.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-prune-react2.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-prune-react3.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-prune-react4.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-create-class-naming.input.js: `Component` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-displayName.input.js: `A` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-displayName.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-displayName.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-displayName.input.js: `` was skipped because of inconvertible mixins.
    console.warn transforms/class.js:1293
      /code/react-codemod/transforms/__testfixtures__/class/class-no-display-name.input.js: `Component` was skipped because of inconvertible mixins.

 PASS  transforms/__tests__/sort-comp-test.js
 PASS  transforms/__tests__/create-element-to-jsx-test.js
 PASS  transforms/__tests__/React-PropTypes-to-prop-types-test.js
 PASS  transforms/__tests__/custom-sort.js
 PASS  transforms/__tests__/react-to-react-dom-test.js
  ● Console

    console.log transforms/react-to-react-dom.js:165
      Using existing ReactDOM var in /code/react-codemod/transforms/__testfixtures__/react-to-react-dom/mixed-with-existing-react-dom.input.js
    console.log transforms/react-to-react-dom.js:165
      Using existing ReactDOM var in /code/react-codemod/transforms/__testfixtures__/react-to-react-dom/import-with-existing-react-dom.input.js

 PASS  transforms/__tests__/pure-render-mixin-test.js
 PASS  transforms/__tests__/ReactNative-View-propTypes-test.js
 PASS  transforms/__tests__/pure-component-test.js
  ● Console

    console.warn transforms/pure-component.js:247
      Class "Impure" skipped in /code/react-codemod/transforms/__testfixtures__/pure-component.input.js on 15:0
    console.warn transforms/pure-component.js:247
      Class "ImpureWithRef" skipped in /code/react-codemod/transforms/__testfixtures__/pure-component.input.js on 24:0
    console.warn transforms/pure-component.js:247
      Class "ImpureClassProperty" skipped in /code/react-codemod/transforms/__testfixtures__/pure-component.input.js on 50:0
    console.warn transforms/pure-component.js:247
      Class "ImpureClassPropertyWithTypes" skipped in /code/react-codemod/transforms/__testfixtures__/pure-component.input.js on 57:0
    console.warn transforms/pure-component.js:247
      Class "Impure" skipped in /code/react-codemod/transforms/__testfixtures__/pure-component2.input.js on 15:0
    console.warn transforms/pure-component.js:274
      Unable to destructure UsesThisDotProps props.
    console.warn transforms/pure-component.js:274
      Unable to destructure DestructuresThisDotProps props.
    console.warn transforms/pure-component.js:274
      Unable to destructure HasShadowProps props.

 PASS  transforms/__tests__/manual-bind-to-arrow-test.js
 PASS  transforms/__tests__/React-DOM-to-react-dom-factories-test.js
 PASS  transforms/__tests__/findDOMNode-test.js
 PASS  transforms/__tests__/error-boundaries.js

Test Suites: 14 passed, 14 total
Tests:       139 passed, 139 total
Snapshots:   0 total
Time:        4.913s
Ran all test suites.
✨  Done in 5.35s.
[After] Example of running `yarn test`
yarn run v1.16.0
$ jest
 PASS  transforms/__tests__/React-DOM-to-react-dom-factories-test.js
 PASS  transforms/__tests__/manual-bind-to-arrow-test.js
 PASS  transforms/__tests__/rename-unsafe-lifecycles-test.js
 PASS  transforms/__tests__/pure-render-mixin-test.js
 PASS  transforms/__tests__/react-to-react-dom-test.js
 PASS  transforms/__tests__/React-PropTypes-to-prop-types-test.js
 PASS  transforms/__tests__/pure-component-test.js
 PASS  transforms/__tests__/ReactNative-View-propTypes-test.js
 PASS  transforms/__tests__/findDOMNode-test.js
 PASS  transforms/__tests__/create-element-to-jsx-test.js
 PASS  transforms/__tests__/error-boundaries.js
 PASS  transforms/__tests__/custom-sort.js
 PASS  transforms/__tests__/class-test.js
 PASS  transforms/__tests__/sort-comp-test.js

Test Suites: 14 passed, 14 total
Tests:       139 passed, 139 total
Snapshots:   0 total
Time:        5.144s
Ran all test suites.
✨  Done in 6.18s.

Update node version in CI

Previously, the node version was set to 4 for CI. This caused CI to fail (see this build) with the error:

$ yarn --version
/usr/local/yarn-latest/lib/cli.js:45699
  let {
      ^
SyntaxError: Unexpected token {
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/usr/local/yarn-latest/bin/yarn.js:24:13)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)

There is an open issue (yarnpkg/yarn#6900) for this when using node v4 with newer versions of yarn. I updated it to version 10 for CI which resolved this error. I based this decision on the Node Releases and that 10 is currently "Active LTS".

Testing these changes

  • After checking out this branch, run yarn to install the new packages
    • Note that the prepublish script is now ran with yarn (rather than npm)
  • Run yarn test
    • Note that there is not verbose custom logging from the various transforms

@skovy skovy mentioned this pull request Jul 14, 2019
3 tasks
@threepointone
Copy link
Contributor

Could you rebase/merge from master?

@threepointone
Copy link
Contributor

Let’s only update package.json in this PR, and then I’ll land it.

@skovy
Copy link
Contributor Author

skovy commented Jul 27, 2019

Thanks for the feedback @threepointone! I believe I reverted all the unnecessary changes and resolved the conflicts 😅

@threepointone
Copy link
Contributor

Thank you very much! I'll merge this and cut a new version. I appreciate the effort a lot!

@threepointone threepointone merged commit c77dc79 into reactjs:master Jul 27, 2019
@skovy skovy deleted the skovy/upgrade-dependencies branch July 27, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants