Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Feb 11, 2019

WHY are these changes introduced?

Part of removing lodash #991

WHAT is this pull request doing?

Creating a less featureful merge function and using that instead of _.merge

How to 🎩

Test pass? 💯

TODO:

  • add changelog

@BPScott
Copy link
Member

BPScott commented Feb 12, 2019

Repeating my query from #1010 here too:

I'm not too familiar with how mergeAppProviderOptions() and legacy context stuff does its thing, but do we need a deep merge there and in the withAppProvider / withSticky HoCs or could we get away with a shallow one using Object.assign()?

@AndrewMusgrave
Copy link
Member Author

Unfortunately merging context requires a deep merge since the user could define partial context and have it collide with polaris.

@solonaarmstrong-zz
Copy link

Elegant! 👌

@AndrewMusgrave AndrewMusgrave force-pushed the rm-lodash-merge branch 3 times, most recently from d63f0db to 9ce3a91 Compare March 14, 2019 22:43
@AndrewMusgrave
Copy link
Member Author

Ready for 👀

@dleroux
Copy link
Contributor

dleroux commented Mar 21, 2019

I rebased it because conflicts with Michelle's autobind.

@dleroux dleroux added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Mar 21, 2019
@dleroux
Copy link
Contributor

dleroux commented Mar 21, 2019

I assume we skip change logs for these?

@dleroux
Copy link
Contributor

dleroux commented Mar 21, 2019

Although I wonder why percy changed. It's not being translated?

@AndrewMusgrave
Copy link
Member Author

I assume we skip change logs for these?

Iv'e been adding them to Code Quality 😄

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

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants