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

Skip install script if already installed #91

Merged
merged 1 commit into from
May 7, 2020
Merged

Conversation

yyx990803
Copy link
Contributor

The install script sometimes throws error on subsequent yarn installs since yarn seems to invoke install scripts when it is shuffling things around. It also causes issue for CI where the node_modules directory is cached across builds.

The install script sometimes throws error on subsequent `yarn` installs since `yarn`
seems to invoke install scripts when it is shuffling things around. It also causes
issue for CI where the `node_modules` directory is cached across builds.
@evanw
Copy link
Owner

evanw commented May 7, 2020

Ah that's annoying. I use npm, not yarn, so I've only been testing with that. You don't happen to have a repro, do you?

I'm wary that this change won't fix the problem. Right after the call to installPackage(), the install script then does a fs.renameSync() of a file inside the installation directory to move it into place. I'm guessing that call will fail if it's already been moved. So I suspect that you'll also have to tell the caller to return early too. But it'd be great to have a repro to double-check that this is correct.

@evanw
Copy link
Owner

evanw commented May 7, 2020

Actually, since it sounds like the postinstall script just needs to be idempotent, this should be something I can test. I'll merge this and fix it up so it works.

@evanw evanw merged commit efc0d0d into evanw:master May 7, 2020
evanw added a commit that referenced this pull request Sep 23, 2020
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