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.

Sign up for GitHub

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

Jump to bottom

fix: Fixed deepmerge module export naming issue #935

Merged
merged 2 commits into from
Jun 3, 2019
Merged

fix: Fixed deepmerge module export naming issue #935

merged 2 commits into from
Jun 3, 2019

Conversation

chrisamador
Copy link
Contributor

Fixes #899

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

This PR fixes webpack's interpolation issue with the deepmerge package. Related to the changes made to deepmerge. Renaming merge to deepmerge fixes the issue.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label May 23, 2019
@pukhalski
Copy link

Can it be merged please?

thinkingserious
thinkingserious requested changes May 28, 2019
View reviewed changes
Copy link
Contributor

@thinkingserious thinkingserious left a comment

Choose a reason for hiding this comment

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

Any thoughts on this one @adamreisnz?

packages/helpers/classes/personalization.js Outdated
@@ -7,7 +7,7 @@ const EmailAddress = require('./email-address');
const toCamelCase = require('../helpers/to-camel-case');
const toSnakeCase = require('../helpers/to-snake-case');
const deepClone = require('../helpers/deep-clone');
const merge = require('deepmerge');
const deepmerge = require('deepmerge');
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, how about deepMerge?

@thinkingserious thinkingserious added difficulty: easy fix is easy in difficulty type: bug bug in the library type: community enhancement feature request not on Twilio's roadmap and removed type: bug bug in the library labels May 28, 2019
@adamreisnz
Copy link
Contributor

Interesting bug, seems like this is something that should be fixed in Webpack ideally, as we should be free to call our variables however we want, without it affecting the build.

But happy with this change if it fixes the issue for the time being, either deepmerge if that's what's necessary to fix it, or otherwise deepMerge as @thinkingserious suggested 👍

@thinkingserious
Copy link
Contributor

Thanks for weighing in Adam!

@thinkingserious thinkingserious merged commit c914d71 into sendgrid:master Jun 3, 2019
@thinkingserious
Copy link
Contributor

Hello @chrisamador,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@CharlBest
Copy link

Hi there.

Is there any chance that this package can be deployed more often and not every 6 months or so as we need some of the bugs fixes. For example my mails are broken because of this issue. Unfortunately I can't downgrade Webpack because I'm using the Angular CLI which has Webpack as a peer dependency which means I have to use the version that they provide. So I'm stuck now and based on the release schedule for this project I'm still going to wait a long time is that right?

Thanks for fixing these issues.

@richardcase
Copy link

I'm in the same situation and would really a appreciate a new release with this fix in.

@adamf
Copy link

adamf commented Oct 15, 2019

Hi! I'm also hitting this bug, and it's preventing us from using sendgrid. Any eta or workaround?

@aymericbouzy
Copy link

@thinkingserious would it be possible to publish a new version with this fix included? It's blocking us as well.

@childish-sambino
Copy link
Contributor

Releasing soon ...

@childish-sambino childish-sambino changed the title Fixed deepmerge module export naming issue fix: Fixed deepmerge module export naming issue Jan 15, 2020
@lentz lentz mentioned this pull request Jan 17, 2020
Merged
@pzhine pzhine mentioned this pull request Feb 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thinkingserious thinkingserious thinkingserious approved these changes

@adamreisnz adamreisnz adamreisnz approved these changes

Assignees
No one assigned
Labels
difficulty: easy fix is easy in difficulty status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

deepmerge needs to be updated to latest version to prevent webpack bug
9 participants
@chrisamador @pukhalski @adamreisnz @thinkingserious @CharlBest @richardcase @adamf @aymericbouzy @childish-sambino