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

Packages: Replace usage of lodash with lodash-es #9374

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@gziolo
Member

gziolo commented Aug 27, 2018

Description

This PR replaces all usage of lodash with lodash-es in app code. It still uses lodash for tools which can't use ES modules.

This aims to improve the size of published packages by using the version of lodash which can be optimized with tools like Webpack using tree shaking.

How has this been tested?

npm test
npm run dev

Types of changes

Refactoring.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo self-assigned this Aug 27, 2018

@gziolo gziolo requested review from youknowriad and aduth Aug 27, 2018

@gziolo gziolo added this to the 3.7 milestone Aug 27, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Aug 27, 2018

Should we update the webpack config to define lodash-es as an external? This ensures we use the lodash script defined in WordPress in WP context and keepp lodash-es for package consumption.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Aug 27, 2018

I'm wondering about the CHANGELOG.md. Should we mention this there as an enhancement or something?

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 27, 2018

Should we update the webpack config to define lodash-es as an external? This ensures we use the lodash script defined in WordPress in WP context and keepp lodash-es for package consumption.

I think we have it covered:
https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L122-L123
https://github.com/WordPress/gutenberg/blob/master/webpack.config.js#L163

I'm wondering about the CHANGELOG.md. Should we mention this there as an enhancement or something?

Yeah, good question :)

@@ -96,6 +96,7 @@
"jest-puppeteer": "3.2.1",
"lerna": "3.0.0-rc.0",
"lint-staged": "7.2.0",
"lodash": "4.17.10",

This comment has been minimized.

@youknowriad

youknowriad Aug 27, 2018

Contributor

Why do we need both?

This comment has been minimized.

@gziolo

gziolo Aug 27, 2018

Member

Node 8 doesn't have ES modules enabled natively so we need to use lodash in all CLI tools.

This comment has been minimized.

@youknowriad

youknowriad Aug 27, 2018

Contributor

Looks like there's some breaking in e2e tests, I guess it's related?

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 27, 2018

e2e tests fail because of the missing lodash-es to lodash mapping ... That's another reason why we need both.

Now that I think about it, we probably should have lodash-es for build-module folder and lodash for build ...

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Aug 27, 2018

Now that I think about it, we probably should have lodash-es for build-module folder and lodash for build ...

I don't think build is targetted for node specifically while build-module is for browsers. I think both are targeted for both environments depending on whether you need to tree shake or not. But that does raise the question about node consumption of our packages. Maybe it's fine to delay this PR until node supports es modules by default?

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 27, 2018

Now that I think about it, we probably should have lodash-es for build-module folder and lodash for build ...

I don't think build is targetted for node specifically while build-module is for browsers. I think both are targeted for both environments depending on whether you need to tree shake or not. But that does raise the question about node consumption of our packages. Maybe it's fine to delay this PR until node supports es modules by default?

build targets ES5 or node? I'm not that sure what is what now :) Anyways, ES5 doesn't use import & export but lodash-es does: https://unpkg.com/lodash-es@4.17.10/lodash.js.

@aduth

This comment has been minimized.

Member

aduth commented Aug 28, 2018

I'd wondered if we could just use an alias such that a developer could import as though from lodash proper, but at build-time it's interpreted to lodash-es if applicable. This could minimize the amount of changes necessary, and avoids confusion around "What is lodash-es?" or cases where we might actually need to use the lodash module proper (maybe related to failures we've observed).

@aduth

This comment has been minimized.

Member

aduth commented Aug 28, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 28, 2018

Yes, I’m coming to the same conclusion after seeing what kind of issues the current proposal creates.

@gziolo gziolo removed this from the 3.7 milestone Aug 29, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 29, 2018

As discussed during the weekly JS chat yesterday, it won't work as intended in the current form. Ideally we should wait for lodash v5 which would ship lodash being the main entry point in package.json and lodash-es being the module.

@aduth

This comment has been minimized.

Member

aduth commented Aug 29, 2018

I experimented with babel-plugin-module-resolver. It works as expected.

The main awkwardness is in how a package would need to define its dependency as lodash-es, while using it as import { foo } from 'lodash';.

diff --git a/packages/babel-preset-default/index.js b/packages/babel-preset-default/index.js
index d62a0c02d..63793941d 100644
--- a/packages/babel-preset-default/index.js
+++ b/packages/babel-preset-default/index.js
@@ -21,6 +21,11 @@ module.exports = function( api ) {
 			} ],
 			'@babel/plugin-proposal-async-generator-functions',
 			! isTestEnv && [ '@babel/plugin-transform-runtime', { corejs: 2 } ],
+			[ 'module-resolver', {
+				alias: {
+					lodash: 'lodash-es',
+				},
+			} ],
 		].filter( Boolean ),
 	};
 };
@aduth

This comment has been minimized.

Member

aduth commented Aug 29, 2018

Should we close this pull request?

@gziolo

This comment has been minimized.

Member

gziolo commented Aug 29, 2018

Yes, it will have to list both lodash versions as dependencies and lodash-es will only be used after being published to npm 😃

@gziolo gziolo closed this Aug 29, 2018

@youknowriad youknowriad deleted the try/lodash-es branch Aug 29, 2018

@ktmn

This comment has been minimized.

ktmn commented Sep 1, 2018

Did something to do with lodash or underscores make it to 3.7?

My custom JS file had some const thing = _.uniq(...); defined but after 3.7 _ was no longer defined in the window at the time, but seems to be made available later.

I fixed it with some webpack config (to use window.lodash as _) but still odd that it stopped working in the first place, since the order in which I enqueue my JS didn't change.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 1, 2018

If you use lodash or underscore in your custom JS, you have to make sure it's a dependency of your custom script.

Before 3.7 Gutenberg was using wp-api which itself was using underscore but now Gutenberg don't need these scripts anymore so we removed them.

So each time you use an external script, don't forget to add it as a dependency to your script to avoid breakage when we update Gutenberg's code.

@ktmn

This comment has been minimized.

ktmn commented Sep 1, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Sep 1, 2018

yes, don't forget the dependencies in your wp script definition, that's it.

@aduth aduth referenced this pull request Nov 27, 2018

Open

API Fetch: Support custom fetch handlers #12365

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment