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

Optimize exports of the wp/compose package #17945

Merged
merged 1 commit into from
Oct 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/compose/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"main": "build/index.js",
"module": "build-module/index.js",
"react-native": "src/index",
"sideEffects": false,
Copy link
Member

Choose a reason for hiding this comment

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

I looked for any things in this package that would violate sideEffects and didn't find any 👍

A "side effect" is defined as code that performs a special behavior when imported, other than exposing one or more exports. An example of this are polyfills, which affect the global scope and usually do not provide an export.

source

"dependencies": {
"@babel/runtime": "^7.4.4",
"@wordpress/element": "file:../element",
Expand Down
14 changes: 14 additions & 0 deletions packages/compose/src/higher-order/compose.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* External dependencies
*/
import { flowRight as compose } from 'lodash';

/**
* Composes multiple higher-order components into a single higher-order component. Performs right-to-left function
* composition, where each successive invocation is supplied the return value of the previous.
*
* @param {...Function} hocs The HOC functions to invoke.
*
* @return {Function} Returns the new composite function.
*/
export default compose;
16 changes: 2 additions & 14 deletions packages/compose/src/index.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,8 @@
/**
* External dependencies
*/
import { flowRight } from 'lodash';

// Utils
export { default as createHigherOrderComponent } from './utils/create-higher-order-component';

/**
* Composes multiple higher-order components into a single higher-order component. Performs right-to-left function
* composition, where each successive invocation is supplied the return value of the previous.
*
* @param {...Function} hocs The HOC functions to invoke.
*
* @return {Function} Returns the new composite function.
*/
export { flowRight as compose };
// Compose helper (aliased flowRight from Lodash)
export { default as compose } from './higher-order/compose';
Copy link
Member

Choose a reason for hiding this comment

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

Question: Was there a reason this couldn't be the following? Would it hinder tree shaking?

Suggested change
export { default as compose } from './higher-order/compose';
export { flowRight as compose } from 'lodash';

I'm not requesting this be changed, it seems fine to me as-is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would also work. I tried to make the index.js file to contain only reexports and nothing else, and move all implementation (It can be argued that the use lodash is an implementation detail) and documentation (I also need to put the jsdoc comment somewhere) to other modules.

Copy link
Member

Choose a reason for hiding this comment

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

I'd love to learn more as well. I'm fine leaving it as is but at the same time I can't figure out why it would make a difference.


// Higher-order components
export { default as ifCondition } from './higher-order/if-condition';
Expand Down