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

Hoodie install breaks on my (admittedly unusual) setup #751

Open
parallaxeffect opened this issue May 29, 2017 · 6 comments
Open

Hoodie install breaks on my (admittedly unusual) setup #751

parallaxeffect opened this issue May 29, 2017 · 6 comments

Comments

@parallaxeffect
Copy link

parallaxeffect commented May 29, 2017

I've been doing my development on Virtual Machines using Vagrant, and I use a symbolic link on the node_modules directory to prevent it from being synced with my host machine. This causes the hoodie install to fail.

Steps to reproduce:

mkdir proxy
mkdir proxy/node_modules
mkdir app
cd app
ln -nsf ../proxy/node_modules node_modules
npm init -y
npm i -S hoodie

The issue is that setup.js uses path.resolve('..', '..') to find the App Root, but that leads to the proxy directory and not the app directory where my package.json lives.
I have a workaround, so it's not a high priority issue for me.
One solution that works for my scenario is to use path.resolve(process.env.PWD, '..', '..') instead to find the App Root, but I'm not sure if that would break under other setups.
In any case, I think the script should fail gracefully if it can't find the package.json where it's expecting. As it is, it's reverting the entire installation after throwing an exception. It should probably just emit a friendly message: "Hey, couldn't find your package.json to add the start script." and exit without reverting the installation.

@gr2m
Copy link
Member

gr2m commented May 29, 2017

I can see the problem. I would probably do path.resolve(process.cwd(), '..', '..') instead of path.resolve(process.env.PWD, '..', '..'), if you could send a PR for that, that’d be great!

In any case, I think the script should fail gracefully if it can't find the package.json where it's expecting.

Yes, I agree. But I’m not sure if that is easily possible. Maybe an error and an process.exit(1) would work, but not sure if it is caught by the npm lifecycle. If you could give it a try, that’d be much appreciated

@parallaxeffect
Copy link
Author

process.cwd() doesn't work. It returns the cwd's "true path": /proxy/node_modules/hoodie, whereas process.env.PWD is set by the shell on start-up and returns the path that the shell currently believes is the cwd: /app/node_modules/hoodie.

I'll try to work on the fail gracefully for now.

@gr2m
Copy link
Member

gr2m commented Jun 4, 2017

@parallaxeffect I just found out that process.env.PWD is not cross OS, see https://stackoverflow.com/questions/44355912/hoodie-postinstall-failed-process-env-pwd-is-undefined

@parallaxeffect
Copy link
Author

Yeah, I don't think PWD is the proper solution for this problem. I was just trying to illustrate that it's the discrepancy between process.env.PWD and the real path that npm uses that is at the heart of the problem.

@gr2m
Copy link
Member

gr2m commented Jun 5, 2017

Would maybe a nested folder setup like described in https://github.com/boennemann/alle and implemented by PouchDB’s monorepo work for your use case?

I think in the future I would get rid of the the postinstall script altogether and create a hoodie CLI tool instead which can do sth like hoodie new myappname and which does the whole setup with npm, package.json and README.md. I guess that will resolve your issue here as well

@parallaxeffect
Copy link
Author

Yeah, my current work-around is to add a symlink in /proxy where npm thinks package.json should be and linking it to my actual package.json in /app. It seems to be working just fine.
I just wanted to make sure you were aware of the issue in case it comes up in some other context or so if someone else comes to you with a similar problem you'll know what's wrong and be able to help them with it.
I agree with the idea of getting rid of the postinstall script in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants