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: #28 #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: #28 #36

wants to merge 1 commit into from

Conversation

seognil
Copy link

@seognil seognil commented Jul 3, 2019

I meet the bug for my tool la-starter-cli

It seems the same as #28

the bug

In my case,
app-root-path parse my tool's appRoot as /usr/local/lib/ on Mac

So I got /usr/local/lib/package.json not found in my code

It should be /usr/local/lib/node_modules/@seognil-lab/la-starter-cli/ instead

(install and run my tool lcli -v)
(the bug should appear after reopen my code line 7 & 8 and comment line 10)

the fix

I trace the code, it happens while there are multi node_modules in the path string

e.g. /usr/local/lib/node_modules/@seognil-lab/la-starter-cli/node_modules/app-root-path/lib

I update the code, and test manually passed for both mac and window

It may fix #28
(Maybe #20 as well, not checked that too much)

remaining

But I'm not sure the origin logic,
And I know app-root-path is used by tons of libs
So I also keep it as comment code,

Please check it
Thanks ~

(UPDATE)
I notice that CI failed,
so maybe we could seperate the use-case somehow?

@inxilpro
Copy link
Owner

Is this still an issue with 3.0.0?

@seognil
Copy link
Author

seognil commented Nov 23, 2019

@inxilpro hi there~
I just tested the v3.0.0, yes it seems still an issue…

notice the case /usr/local/lib/node_modules/@seognil-lab/la-starter-cli/node_modules/app-root-path/lib
there are two node_modules, and the second one is target

seems to need a better way to decide, maybe could use some node.js runtime flag such as __dirname 🤔?

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.

Incorrect root path for app globally installed on Mac
2 participants