Skip to content

Conversation

@LuckyPigeon
Copy link

No description provided.

@LuckyPigeon
Copy link
Author

related to #1593

Copy link
Member

@implausible implausible left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Author

@LuckyPigeon LuckyPigeon left a comment

Choose a reason for hiding this comment

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

@implausible Updated

@LuckyPigeon
Copy link
Author

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

@LuckyPigeon
Copy link
Author

@implausible Updated

@implausible
Copy link
Member

implausible commented Feb 21, 2019

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
Copy link
Member

implausible commented Mar 4, 2019

Closed in favor of #1644

@implausible implausible closed this Mar 4, 2019
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