[codemod] Add codemods for optimal tree-shakeable imports #16192

jedwards1211 · 2019-06-12T20:46:19Z

I have followed (at least) the PR section of the contributing guide.

eps1lon · 2019-06-12T20:57:50Z

We don't need this codemod for anything below 4.x. I think this is currently a bit overzealous. @material-ui/core/styles is totally fine.

A good heuristic would not change any code documented in our docs.

jedwards1211 · 2019-06-12T21:03:46Z

@eps1lon I'm planning to make a separate PR for a codemod that preserves 2nd-level subpaths. Having multiple options seems to be leading to unnecessary debate about where to import from. If you don't want this one then I'll have to publish it in its own repository.

jedwards1211 · 2019-06-12T21:05:32Z

Also, would this not be useful for 3.x? I assume that the tree-shaking problem applies to at least 3.x as well as 4.x.

eps1lon · 2019-06-12T21:10:50Z

I assume that the tree-shaking problem applies to at least 3.x as well as 4.x.

Yeah now that I think about this might be better since 3.x only has tree-shaking for top level imports. Only 4.x supports tree shaking for second level imports.

We should test this on the 3.x docs code and see if it still compiles though.

It needs an exception for test-utils. Those are not part of the top level export.

mui-pr-bot · 2019-06-12T21:21:36Z

No bundle size changes comparing 2853239...a0ef850

Generated by 🚫 dangerJS against a0ef850

jedwards1211 · 2019-06-12T21:37:30Z

@eps1lon theoretically 3.x should be handled properly and test-utils should be excluded already because of how the codemod builds a whitelist of all exported identifiers in @material-ui/core/index.d.ts.

The separate 2nd-level-imports codemod I'm making (maybe I should call it 4.x/optimal-imports.js? Will have to have case-by-case logic to exclude internal and test-utils.

jedwards1211 · 2019-06-12T22:18:14Z

Okay I rebased this onto v3.x, transformed the docs and I was able to run the documentation site with no compile errors.

I had to do the following hack to get the path to index.d.ts in test mode; do you know of any better way to do this? All I can think of would be using babel-plugin-module-resolver in test mode...

  const whitelist = getTSExports(process.env.TEST
    ? require.resolve('../../../material-ui/src/index.d.ts')
    : require.resolve(`${importModule}/index.d.ts`, {
      paths: [dirname(fileInfo.source)],
    })
  );

eps1lon · 2019-06-12T22:27:08Z

I don't think this should parse the TS files. For one it requires @babel/preset-typescript. The other issue is that types are an afterthought i.e. might be outdated (we're getting better). Can't you just use index.js at which point you can safely leverage @material-ui/core/package.json and the main entry.

oliviertassinari · 2019-06-12T22:42:37Z

Do we all agree that this pull request should target the master branch?

jedwards1211 · 2019-06-12T22:45:40Z

@oliviertassinari as discussed above this codemod is useful for 3.x...but maybe you have better ideas about how to release it.

jedwards1211 · 2019-06-12T22:48:31Z

@eps1lon that's a good point, I'll do that

oliviertassinari · 2019-06-12T22:50:29Z

I don't understand the logic. From what I understand:

v4 supports tree-shaking, v3 doesn't. I wish we could apply this codemod to all the demos of the master branch.
We don't optimize for v3 users.

eps1lon · 2019-06-12T22:56:30Z

It should target master. The codemod however should only be used for 3.x and below. I meant we should only test it manually on 3.x not target at 3.x

jedwards1211 · 2019-06-12T22:56:46Z

@oliviertassinari I'm so confused, v3.x has "sideEffects": false, so shouldn't at least some tree shaking work?

jedwards1211 · 2019-06-12T23:03:38Z

okay rebased onto master

eps1lon · 2019-06-12T23:26:27Z

import Button from '@material-ui/core/Button' will point to a commonJS build in 3.x. Tree-shaking is not supported for those modules (webpack 4 default).

jedwards1211 · 2019-06-12T23:44:10Z

@eps1lon gotcha. I just added a v4.0.0/optimal-imports.js codeshift that preserves 1 level deep like: @material-ui/core/styles

jedwards1211 · 2019-06-12T23:46:03Z

let me know if there are any other codemods I could help write...I love writing codemods

jedwards1211 · 2019-06-13T01:23:14Z

not sure what's up with the argos difference

eps1lon

Test look good to me. Just some minor questions/suggestions about the implementation.

packages/material-ui-codemod/README.md

packages/material-ui-codemod/src/util/getJSExports.js

eps1lon · 2019-06-13T09:32:32Z

packages/material-ui-codemod/src/v4.0.0/optimal-imports.js

+import addImports from 'jscodeshift-add-imports';
+
+// istanbul ignore next
+if (process.env.NODE_ENV === 'test') {


We shouldn't publish env branches unless they compare with production.

You should be able to get the esModules index barrel by reading @material-ui/core/package.json and using the module entry. Then you can use main as a fallback to account for test runs in the monorepo.

I don't understand about branches, i'm checking if it's test mode, not if it's a branch

Oh you mean a branch statement, not a code branch.

How do you want me to get the import path in testing? babel-plugin-module-resolver doesn't work for for dynamic require.resolve statements. Mocking require.resolve outside of this file won't work AFAIK. Maybe using a define plugin to inject some code in test mode?

Btw reading the package json and using it in require.resolve on subpaths still won't work in test mode. Unlike the v3 codemod, this one parses the 2nd-level files to build whitelists, rather than the top-level file

eps1lon · 2019-06-13T09:33:54Z

packages/material-ui-codemod/src/util/memoize.js

@@ -0,0 +1,12 @@
+const memoize = (func, resolver = a => a) => {


yarn add -D -W memoize-one

It's already in node_modules from 4 other dependencies

Unfortunately memoize-one is not great for this, as it only remembers the most recent arguments. With this impl I assume cached info from parsing will stay in memory from one file being transformed to the next. (If passing a dir to jscodeshift instead of the strange usage of find/xargs, at least)

Code size isn't a major concern for the codemods package is it?

eps1lon · 2019-06-13T09:43:32Z

packages/material-ui-codemod/src/util/getJSExports.js

@@ -0,0 +1,22 @@
+import memoize from './memoize';
+import { readFileSync } from 'fs';
+import { parseSync } from '@babel/core';


Can we use the jscodeshift API instead i.e. api.jscodeshift(muiCoreIndexSource).findExportSpecifier() (no idea if that find method exists). I guess it's not guarenteed that the parser config of the user can also read node_modules source but that's also not guarenteed in this form since we don't publish our babel config anyway.

I tried that, got parse errors on the export from statements...perhaps I could build this list at compile time?

packages/material-ui/package.json doesn't have module or main entries, otherwise i would have done what you suggest. Should i add them? Or do they get added at build time?

Since the babel config is not published, is it up to someone importing from es/ to set the right babel options, or do you transpile at least things like export from statements in the build?

@eps1lon I still want to finish up this PR, how do you want me to resolve this?

packages/material-ui-codemod/README.md

oliviertassinari · 2019-07-21T22:07:37Z

Rebased.

oliviertassinari

I have rebased on the master branch, fix the markdown and move the two codemod under the v4 folder. From my end, it's good enough to ship and to put between our user's hand.

Blinnikov · 2019-07-29T10:51:08Z

Guys, I'm a little bit confused with those two options.
As I understand both optimal-imports and top-level-imports allow me to have tree-shakeable imports.
But what is the preferable way to import material-ui modules?
From reading the Docs I can only conclude that by default I should use top-level-imports, but if it's a problem in development I can switch on using of optimal-imports. But what are the downsides of optimal-imports in this case? Could you please clarify?
Thanks.

oliviertassinari · 2019-07-29T15:53:14Z

@Blinnikov To sum-up:

optimal-imports cons:

Duplication of the module name. I don't know you but with this approach, to use a new module, I duplicate an existing import line and replace the two occurrences of the copoed module name. Sometimes, I only replace one by mistake. Boy, good luck figuring out the source of your problem when that happens.
Verbose.

top-level-import cons:

No TypeScript support for the xProps types yet.
Can be slow when a lot of modules are exported, e.g. with the icons package (5k modules).

eps1lon · 2019-07-29T16:22:52Z

Can be slow when a lot of modules are exported, e.g. with the icons package (5k modules).

To clarify: This is slow for cold starts of development builds or when building for production. It has no impact on the consumer of your production bundles. This situation will improve the more advanced build caches get (webpack is currently working on it).

jedwards1211 changed the title ~~[IconButton] add shortcut prop for applying negative margins on the left or right~~ add top-level-imports codemod Jun 12, 2019

jedwards1211 closed this Jun 12, 2019

jedwards1211 reopened this Jun 12, 2019

jedwards1211 force-pushed the codemod/top-level-imports branch from 7d24e4b to 2abc72e Compare June 12, 2019 21:02

jedwards1211 force-pushed the codemod/top-level-imports branch from 2abc72e to 80bb909 Compare June 12, 2019 21:07

eps1lon added package: codemod Specific to @mui/codemod new feature New feature or request labels Jun 12, 2019

jedwards1211 force-pushed the codemod/top-level-imports branch from 555f43e to 9cd40f6 Compare June 12, 2019 22:17

jedwards1211 changed the base branch from master to v3.x June 12, 2019 22:17

jedwards1211 force-pushed the codemod/top-level-imports branch from 9cd40f6 to 5b2ac0c Compare June 12, 2019 23:01

jedwards1211 changed the base branch from v3.x to master June 12, 2019 23:03

eps1lon self-requested a review June 13, 2019 00:10

eps1lon changed the title ~~add top-level-imports codemod~~ [codemod] Add codemods for tree-shakeable imports Jun 13, 2019

eps1lon changed the title ~~[codemod] Add codemods for tree-shakeable imports~~ [codemod] Add codemods for optimal tree-shakeable imports Jun 13, 2019

eps1lon requested changes Jun 13, 2019

View reviewed changes

oliviertassinari reviewed Jun 13, 2019

View reviewed changes

packages/material-ui-codemod/README.md Outdated Show resolved Hide resolved

oliviertassinari mentioned this pull request Jul 18, 2019

Duplicated MuiButtonBase (and others) in <style> elements #15610

Closed

2 tasks

oliviertassinari force-pushed the codemod/top-level-imports branch 2 times, most recently from d2dafaa to 51a064a Compare July 21, 2019 22:26

mui deleted a comment from jedwards1211 Jul 21, 2019

oliviertassinari force-pushed the codemod/top-level-imports branch 5 times, most recently from 55946a3 to 2237fe0 Compare July 21, 2019 22:57

oliviertassinari approved these changes Jul 21, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Jul 22, 2019

oliviertassinari force-pushed the codemod/top-level-imports branch from 4a0d3f8 to 604fffe Compare July 24, 2019 09:25

feat(codemod): create top-level-imports codemod

a0ef850

oliviertassinari force-pushed the codemod/top-level-imports branch from 604fffe to a0ef850 Compare July 24, 2019 10:09

oliviertassinari merged commit fd24690 into mui:master Jul 24, 2019

oliviertassinari mentioned this pull request Jul 29, 2019

docs: named imports #16803

Closed

snyk-bot mentioned this pull request Sep 4, 2019

[Snyk] Upgrade @material-ui/styles from 4.3.0 to 4.3.3 iliutastoica/React-Discord-Clone#5

Open

		@@ -0,0 +1,12 @@
		const memoize = (func, resolver = a => a) => {

[codemod] Add codemods for optimal tree-shakeable imports #16192

[codemod] Add codemods for optimal tree-shakeable imports #16192

Conversation

jedwards1211 commented Jun 12, 2019

eps1lon commented Jun 12, 2019

jedwards1211 commented Jun 12, 2019 • edited Loading

jedwards1211 commented Jun 12, 2019

eps1lon commented Jun 12, 2019

mui-pr-bot commented Jun 12, 2019 • edited Loading

jedwards1211 commented Jun 12, 2019 • edited Loading

jedwards1211 commented Jun 12, 2019 • edited Loading

eps1lon commented Jun 12, 2019

oliviertassinari commented Jun 12, 2019

jedwards1211 commented Jun 12, 2019 • edited Loading

jedwards1211 commented Jun 12, 2019

oliviertassinari commented Jun 12, 2019 • edited Loading

eps1lon commented Jun 12, 2019

jedwards1211 commented Jun 12, 2019

jedwards1211 commented Jun 12, 2019

eps1lon commented Jun 12, 2019 • edited Loading

jedwards1211 commented Jun 12, 2019

jedwards1211 commented Jun 12, 2019

jedwards1211 commented Jun 13, 2019

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedwards1211 Jun 13, 2019 • edited Loading

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

jedwards1211 Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Jul 21, 2019

oliviertassinari left a comment

Choose a reason for hiding this comment

Blinnikov commented Jul 29, 2019

oliviertassinari commented Jul 29, 2019 • edited Loading

eps1lon commented Jul 29, 2019

jedwards1211 commented Jun 12, 2019 •

edited

Loading

mui-pr-bot commented Jun 12, 2019 •

edited

Loading

jedwards1211 commented Jun 12, 2019 •

edited

Loading

jedwards1211 commented Jun 12, 2019 •

edited

Loading

jedwards1211 commented Jun 12, 2019 •

edited

Loading

oliviertassinari commented Jun 12, 2019 •

edited

Loading

eps1lon commented Jun 12, 2019 •

edited

Loading

jedwards1211 Jun 13, 2019 •

edited

Loading

jedwards1211 Jun 13, 2019 •

edited

Loading

oliviertassinari commented Jul 29, 2019 •

edited

Loading