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

Framework: Add babel-plugin-lodash to enable root Lodash imports #6539

Merged
merged 4 commits into from
Jul 6, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jul 5, 2016

Related: #3015, #735

This pull request seeks to include babel-plugin-lodash in our Babel configuration to allow for root Lodash imports without worrying about unnecessarily bloating the size of resulting bundles.

Before / After reflects build without Babel plugin, then build with plugin and sites selectors dependency revisions (4995c88).

Before After
before after

Testing instructions:

May be worth having a second set of eyes on before/after build sizes to confirm that no bundle bloating occurs. Verify that no regressions occur in usage of application (sites selectors are pretty pervasive through the app, notably My Sites section).

Test live: https://calypso.live/?branch=add/babel-plugin-lodash

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 5, 2016
@nylen
Copy link
Contributor

nylen commented Jul 6, 2016

LGTM. I would update npm-shrinkwrap.json to only contain the changes related to this PR - I did this in #6543 - but then I noticed that nothing in our shrinkwrap documentation would prevent a full update. Thoughts on which approach is preferred? It seems desirable to avoid potential breakage from changing versions of sub-dependencies.

@aduth
Copy link
Contributor Author

aduth commented Jul 6, 2016

I would update npm-shrinkwrap.json to only contain the changes related to this PR

Generally, I'd agree that including unrelated changes should be avoided. That said, I think it's been more-or-less standard practice that producing a new shrinkwrap will include any version bumps of dependency "grandchildren" (dependency dependencies). Two reasons I see are:

I think this has come up at least once before recently, so whichever process we adopt, I think it'd be good to document in our shrinkwrap docs (cc @gwwar ).

@gwwar
Copy link
Contributor

gwwar commented Jul 6, 2016

I think this has come up at least once before recently, so whichever process we adopt, I think it'd be good to document in our shrinkwrap docs

I'll spin up a PR to update docs. I think it's OK to have sub sub dependencies update. We pick up bug fixes, and these new package versions get tested a bit in the PR. In addition to this, npm 3 tries to create an "ideal" flat tree, so this may mean that we can have a smaller shrinkwrap file if multiple packages happen to use the same sub dependency versions.

@aduth aduth force-pushed the add/babel-plugin-lodash branch from fc26e18 to 3d5e4d2 Compare July 6, 2016 18:57
@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016
@aduth aduth merged commit 4f02000 into master Jul 6, 2016
@aduth aduth deleted the add/babel-plugin-lodash branch July 6, 2016 19:04
@gwwar gwwar mentioned this pull request Sep 2, 2016
obenland added a commit that referenced this pull request Oct 6, 2016
* First pass at wiring up reduxified country states.

See #7922.

* Second pass at wiring up reduxified country states.

* Uses Redux based Google Analytics.
* Moves `QueryCountryStates` inline.
* Removes `lib/states-list`.

* Import `isEmpty` from root module directly.

See #6539.
See https://github.com/Automattic/wp-calypso/pull/8414/files#r81896999

* Properly bind and dispatch state select click.

See https://github.com/Automattic/wp-calypso/pull/8414/files#r81897352

* Add id attribute so `FormLabel` works with the element.

See
#8414 (comment)

* Use a unique id for `FormLabel`

See
#8414 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants