-
Notifications
You must be signed in to change notification settings - Fork 15
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: maximum call stack size exceeded at paths.js (findYarnLock function) #811
fix: maximum call stack size exceeded at paths.js (findYarnLock function) #811
Conversation
Thank you for this PR @CelsoDeCarvalho ! It generally looks good. Could you change it to use |
It is unnecessary to use |
Do you think that I needa change it? |
Thanks Celso - thinking about this a little more, I think using I would recommend instead changing this line to Let me know if this makes sense to you, thanks again for helping to fix this! |
Hi @CelsoDeCarvalho thanks again for your contribution here! Do you think you will have time to apply the changes requested? I'd like to get this fixed, so if you don't have time I can also do it myself and merge a separate PR. Let me know! |
Thanks @amcgee for text me. Oh! I had forgotten about this point. I'll make changes today. Thanks It will be a pleasure to contribute to this repo. |
…inside of recursive function(findYarnLock)
@amcgee This make sense. The variable declaration inside a recursive function can decrease the perfomance. I have made the changes. But for the first point, can you please explain it? |
cli/src/lib/paths.js
Outdated
// Get the root path or drive based on the operating system | ||
const rootPathByOS = path.parse(cwd).root; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer dependent on the operating system, it should work for all operating systems, so we should remove this comment and change the name to simply rootDirectory
// Get the root path or drive based on the operating system | |
const rootPathByOS = path.parse(cwd).root; | |
const rootDirectory = path.parse(cwd).root; |
@CelsoDeCarvalho thanks and certainly! Basically, the variable
In this case, This change looks good to me now! I made one very minor suggestion to update the naming of the root variable. Thanks again for your contribution! |
Don't worry about the commit message lint issue - it's caused by the fact that several commits here aren't exactly following conventional commit standards - 6b61c07 is missing a I will squash-merge this branch when I merge it, so no need to fix those on your branch :-) |
Hi @amcgee So Lemme see if I get it. Basically you saying that there will be possible to create app in a directory passed through the command line in I have made the changes. |
…ory' and remove unnecessary comment
That's correct, thank you @CelsoDeCarvalho ! |
I manually built, tested, and linted this PR before merging. Thanks again @CelsoDeCarvalho for the contribution!! |
## [10.3.10](v10.3.9...v10.3.10) (2023-08-21) ### Bug Fixes * support yarn.lock discovery on non-unix ([#811](#811)) ([22a6863](22a6863))
🎉 This PR is included in version 10.3.10 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
# [10.4.0-alpha.4](v10.4.0-alpha.3...v10.4.0-alpha.4) (2023-08-22) ### Bug Fixes * support yarn.lock discovery on non-unix ([#811](#811)) ([22a6863](22a6863))
🎉 This PR is included in version 10.4.0-alpha.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
The line if(base==="/") I think we are checking only for mac or linux OS 'cause in windows, is if(base==="C:\\") .
I tried this solution and it worked for me.