Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codemods: CommonJS -> ES2015 module #12520

Merged
merged 6 commits into from
Apr 18, 2017
Merged

Conversation

jsnmoon
Copy link
Contributor

@jsnmoon jsnmoon commented Mar 24, 2017

Part of #11688.

This change adds executable codemod scripts to bin/ and bin/codemods. The intent is to have these executables be available for Calypso contributors who are working on migrating CJS modules to ES modules. Each script in bin/codemods/ are individually executable.

Todo:

  • Await merge & publish of Transform called CommonJS imports 5to6/5to6-codemod#40 (1).
  • Fork 5to6-codemod for internal use
  • Add import hoisting script
  • Fix errors for name export generation codemod
  • Fix errors for import codemod
  • Enable useTabs and arrayBracketSpacing Recast option flags
  • Thoroughly test codemods on all *.js and *.jsx files in client/

Caveats:

  • recast improperly handles tabs (recast #185 and #315)
  • recast adds unnecessary newline between first and second import statements (recast #371)

@jsnmoon jsnmoon self-assigned this Mar 24, 2017
@jsnmoon jsnmoon force-pushed the add/cjs-to-es2015-modules-transform branch 3 times, most recently from a08dab1 to 0ae0654 Compare March 24, 2017 20:23
@jsnmoon jsnmoon force-pushed the add/cjs-to-es2015-modules-transform branch from 0ae0654 to cef3540 Compare March 24, 2017 20:45
package.json Outdated
@@ -154,6 +154,7 @@
"css-lint": "stylelint 'client/**/*.scss' --syntax scss"
},
"devDependencies": {
"5to6-codemod": "^1.5.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pin these versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@ehg
Copy link
Member

ehg commented Mar 27, 2017

Nice work. Pleased to see this coming together!

@jsnmoon jsnmoon changed the title Add CommonJS to ES2015 modules codemods Add codemods: CommonJS -> ES2015 module Mar 27, 2017
@ockham
Copy link
Contributor

ockham commented Mar 27, 2017

I've just ran bin/codemods/commonjs-imports on client/my-sites/pages/. It looks like the codemod doesn't respect the External / Internal dependencies comment blocks; all but one import end up above the corresponding comment block, and only the last one stays below.

Like so:

diff --git a/client/my-sites/pages/page-list.jsx b/client/my-sites/pages/page-list.jsx
index 81b1424..59ae927 100644
--- a/client/my-sites/pages/page-list.jsx
+++ b/client/my-sites/pages/page-list.jsx
@@ -1,22 +1,24 @@
+import React from 'react';
+import PureRenderMixin from 'react-pure-render/mixin';
+
 /**
  * External dependencies
  */
-var React = require( 'react' ),
-       PureRenderMixin = require( 'react-pure-render/mixin' ),
-       omit = require( 'lodash/omit' );
+import omit from 'lodash/omit';
+
+import PostListFetcher from 'components/post-list-fetcher';
+import Page from './page';
+import infiniteScroll from 'lib/mixins/infinite-scroll';
+import observe from 'lib/mixins/data-observe';
+import EmptyContent from 'components/empty-content';
+import NoResults from 'my-sites/no-results';
+import actions from 'lib/posts/actions';
+import Placeholder from './placeholder';
 
 /**
  * Internal dependencies
  */
-var PostListFetcher = require( 'components/post-list-fetcher' ),
-       Page = require( './page' ),
-       infiniteScroll = require( 'lib/mixins/infinite-scroll' ),
-       observe = require( 'lib/mixins/data-observe' ),
-       EmptyContent = require( 'components/empty-content' ),
-       NoResults = require( 'my-sites/no-results' ),
-       actions = require( 'lib/posts/actions' ),
-       Placeholder = require( './placeholder' ),
-       mapStatus = require( 'lib/route' ).mapPostStatus;
+import { mapPostStatus as mapStatus } from 'lib/route';

@jsnmoon
Copy link
Contributor Author

jsnmoon commented Mar 27, 2017

@ockham Ah, I'll look into this.

Edit: I opened a PR that fixes this issue.

@jsnmoon jsnmoon force-pushed the add/cjs-to-es2015-modules-transform branch from cef3540 to 65be0b6 Compare March 27, 2017 23:45
@jsnmoon
Copy link
Contributor Author

jsnmoon commented Mar 27, 2017

Hi @ockham, I updated this branch to use my forked 5to6-codemods which includes the most up-to-date fixes for the import transformation. Let me know if the fixes are to your liking!

@jsnmoon jsnmoon added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Blocked / Hold labels Mar 27, 2017
@jsnmoon jsnmoon force-pushed the add/cjs-to-es2015-modules-transform branch from dff635c to 5bdaf2b Compare March 27, 2017 23:57
@ockham
Copy link
Contributor

ockham commented Mar 28, 2017

That's a lot better, they're below the corresponding comment block now. There's still a funny newline after the first import of each block (is that related to the insertAfter bug?)

diff --git a/client/my-sites/pages/page-list.jsx b/client/my-sites/pages/page-list.jsx
index 81b1424..93a46ac 100644
--- a/client/my-sites/pages/page-list.jsx
+++ b/client/my-sites/pages/page-list.jsx
@@ -1,22 +1,24 @@
 /**
  * External dependencies
  */
-var React = require( 'react' ),
-       PureRenderMixin = require( 'react-pure-render/mixin' ),
-       omit = require( 'lodash/omit' );
+import React from 'react';
+
+import PureRenderMixin from 'react-pure-render/mixin';
+import omit from 'lodash/omit';
 
 /**
  * Internal dependencies
  */
-var PostListFetcher = require( 'components/post-list-fetcher' ),
-       Page = require( './page' ),
-       infiniteScroll = require( 'lib/mixins/infinite-scroll' ),
-       observe = require( 'lib/mixins/data-observe' ),
-       EmptyContent = require( 'components/empty-content' ),
-       NoResults = require( 'my-sites/no-results' ),
-       actions = require( 'lib/posts/actions' ),
-       Placeholder = require( './placeholder' ),
-       mapStatus = require( 'lib/route' ).mapPostStatus;
+import PostListFetcher from 'components/post-list-fetcher';
+
+import Page from './page';
+import infiniteScroll from 'lib/mixins/infinite-scroll';
+import observe from 'lib/mixins/data-observe';
+import EmptyContent from 'components/empty-content';
+import NoResults from 'my-sites/no-results';
+import actions from 'lib/posts/actions';
+import Placeholder from './placeholder';
+import { mapPostStatus as mapStatus } from 'lib/route';
 
 import BlogPostsPage from './blog-posts-page';

@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 29, 2017
@jsnmoon
Copy link
Contributor Author

jsnmoon commented Mar 29, 2017

Yup, that's definitely the same bug :)

jsnmoon added a commit that referenced this pull request Apr 13, 2017
Using jscodeshift's `useTabs` and `arrayBracketSpacing` option flags will generate code more in line with our style guide.
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
jsnmoon added a commit that referenced this pull request Apr 13, 2017
@ehg
Copy link
Member

ehg commented Apr 13, 2017

Added run-codemods!

Cool, could we split this up into a separate PR, and at least have the import and export mods available to use manually?

@jsnmoon
Copy link
Contributor Author

jsnmoon commented Apr 17, 2017

@ehg: You got it! I'll split out run-codemods into a separate PR.

EDIT: #13163 has been opened for run-codemods script.

@ehg
Copy link
Member

ehg commented Apr 18, 2017

You got it! I'll split out run-codemods into a separate PR.

Great! Thanks. I'll merge this, and we'll test them out :)

@ehg ehg merged commit 4832b8e into master Apr 18, 2017
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 18, 2017
@jsnmoon jsnmoon deleted the add/cjs-to-es2015-modules-transform branch August 14, 2017 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants