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

fix: Replace a few more uses of lodash #1916

Merged
merged 4 commits into from Feb 19, 2020
Merged

Conversation

paulmelnikow
Copy link
Member

Ref #1285

this.options = {
...this.scope.scopeOptions,
...this.options,
}
Copy link
Member

Choose a reason for hiding this comment

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

Are these guaranteed to be shallow objects?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure, though I think this is the same behavior as _.defaults.

Copy link
Member

Choose a reason for hiding this comment

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

yes, your change is parity.
But I'm questioning if that is correct. Some of the options are objects, eg reqheaders. So I think it's a bug that it doesn't do deep cloning.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's possible. I don't know what the use cases are for merging these options. I'm not sure if it's possible that reqheaders could end up in both (or if merging them would be the correct thing to do). Would you like me to add a todo comment?

Copy link
Member

Choose a reason for hiding this comment

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

hmmm, yeah it's a weird undocumented case. I think I'd vote for keeping with your current change since it keeps parity and come back to it again if anyone ever opens an issue.

@paulmelnikow paulmelnikow changed the title Replace a few more uses of lodash fix: Replace a few more uses of lodash Feb 19, 2020
@paulmelnikow paulmelnikow merged commit 1f47f1a into master Feb 19, 2020
@paulmelnikow paulmelnikow deleted the even-more-lodash branch February 19, 2020 18:53
@github-actions
Copy link

🎉 This PR is included in version 12.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@paulmelnikow
Copy link
Member Author

paulmelnikow commented Feb 19, 2020

I marked this a fix because the _.isString change should have been made as part of the breaking changes in 12.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants