FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add yarn install by LuckyPigeon · Pull Request #1612 · nodegit/nodegit · GitHub

Add yarn install - #1612

Closed
LuckyPigeon wants to merge 6 commits into
nodegit:masterfrom
LuckyPigeon:preinstall-alternative
Closed

Add yarn install#1612
LuckyPigeon wants to merge 6 commits into
nodegit:masterfrom
LuckyPigeon:preinstall-alternative

Conversation

Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown
Author

related to #1593

implausible left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your help! Would you please fix the indentation in these in your promise methods and consider adjusting the flow of your promise chain as I've suggested in my review?

LuckyPigeon left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@implausible Updated

Copy link
Copy Markdown
Author

@implausible I've seen the change requested. I not entirely understand what your "fix the indentation" means, could you explain it more precisely?

Comment thread lifecycleScripts/preinstall.js Outdated
Comment thread lifecycleScripts/preinstall.js Outdated

Copy link
Copy Markdown
Author

@implausible Updated

implausible commented Feb 21, 2019
edited
Loading

Copy link
Copy Markdown
Member

Hrmm.. So upon further review of this code, I noticed that the current patch is proposing to run yarn install --ignore-scripts, but I think this is actually irrelevant.

If you follow the code, we perform npm -v to check if the version of npm is less than 2, and then we run npm install --ignore-scripts. For all versions npm@3 and above, we don't run an install script.

I believe that yarn's behavior will mirror that of npm@3's behavior, and we shouldn't need to run yarn install --ignore-scripts at all.

I think the patch that we're actually looking for looks like this:
implausible@4a1aaf7

Can you confirm this solves your issue?

implausible commented Mar 4, 2019
edited
Loading

Copy link
Copy Markdown
Member

Closed in favor of #1644

implausible closed this Mar 4, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

2 participants


Back | FazBrowse Home | New Git URL