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: add undeclared dependency lodash #43

Merged

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Jul 17, 2021

What's the problem this PR addresses?

scss-parser depends on lodash but doesn't declare it as a dependency

How did you fix it?

Added lodash as a dependency

@jcreamer898
Copy link

Can we get this merged? This is breaking some of our builds where we rely on "strictness" i.e. all packages requiring their dependencies to be declared. Thanks so much!

@jcreamer898
Copy link

@alexiscordova think you can take a look? Thanks!

@merceyz
Copy link
Contributor Author

merceyz commented Sep 14, 2021

Yarn v3 ships with this fix builtin, if you're on v2 you can use https://yarnpkg.com/configuration/yarnrc#packageExtensions, and if you're using pnpm you can use https://pnpm.io/package_json#pnpmpackageextensions

Copy link
Contributor

@alexiscordova alexiscordova left a comment

Choose a reason for hiding this comment

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

One small change and I can get this in. Thank you!

package.json Outdated
@@ -19,7 +19,8 @@
"lint": "standard"
},
"dependencies": {
"invariant": "2.2.4"
"invariant": "2.2.4",
"lodash": "^4.17.21"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you can remove the ^ character, this should be good to go

Choose a reason for hiding this comment

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

@merceyz 👆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get why you want that but sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Internally we use specific version numbers for security purposes. It's something we're hoping to change in the future once we can convince the right teams, but for the time being this is the approach we have to take. Thanks for making this change!

@jcreamer898
Copy link

Are we going to be able to get this merged now? This and the other one salesforce-ux/query-ast#25 too. Thanks!

@iclanton
Copy link

iclanton commented Dec 8, 2022

@alexiscordova - can you follow-up on this?

@alexiscordova
Copy link
Contributor

@iclanton I don't work for Salesforce anymore, but @Dottenpixel might be able to help

@Dottenpixel Dottenpixel merged commit 8ca6819 into salesforce-ux:master Dec 8, 2022
@Dottenpixel
Copy link
Contributor

Thanks for your contribution @merceyz! We'll look to publish a new NPM package version asap.

@merceyz
Copy link
Contributor Author

merceyz commented Dec 9, 2022

No rush on my end, Yarn has been shipping this fix for a while.
Ref yarnpkg/berry#3152

@merceyz merceyz deleted the merceyz/fix/missing-lodash branch December 9, 2022 00:04
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.

None yet

5 participants