Skip to content

Conversation

AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

We shouldn't import packages that aren't dependencies. Lodash isn't a dep. https://github.com/Shopify/polaris-react/blob/master/examples/create-react-app/package.json

Part of #991

WHAT is this pull request doing?

Adding lodash

TODO:

  • add changelog

How to 🎩

Run the example 👍

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-1010 February 11, 2019 21:59 Inactive
@BPScott
Copy link
Member

BPScott commented Feb 12, 2019

I'm not too familar with how mergeAppProviderOptions() and legacy context stuff does its thing, but do we need a deep merge there or could we get away with a shallow one using Object.assign()? If we can do that we wouldn't need this dependency.

EDIT: answered in another PR: we do need a deep merge for this

"private": true,
"dependencies": {
"@shopify/polaris": "^2.2.0",
"lodash": "^4.17.11",
Copy link
Contributor

Choose a reason for hiding this comment

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

🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢

@solonaarmstrong-zz
Copy link

shrink ray needs a rerun here too

@AndrewMusgrave AndrewMusgrave force-pushed the example-cra-remove-merge branch from aaeaed8 to 439b696 Compare February 26, 2019 18:57
@BPScott BPScott temporarily deployed to polaris-react-pr-1010 February 26, 2019 18:57 Inactive
@AndrewMusgrave AndrewMusgrave force-pushed the example-cra-remove-merge branch from 439b696 to b99f9f5 Compare February 26, 2019 20:00
@AndrewMusgrave AndrewMusgrave merged commit 19a0863 into master Feb 26, 2019
@AndrewMusgrave AndrewMusgrave deleted the example-cra-remove-merge branch February 26, 2019 20:09
@amrocha amrocha temporarily deployed to production March 7, 2019 22:06 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants