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 script fails on Windows #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rishi-raj-jain
Copy link

@rishi-raj-jain rishi-raj-jain commented Sep 21, 2024

fixes #37

the fix uses unlink-self if available via node, so that it can run agnostic to the OS

@imballinst
Copy link

imballinst commented Sep 22, 2024

I noticed that the only time node-read-yaml is required is in here, in a test file that would never be run by a consumer:

const read = require('node-read-yaml');

That being the case, I think it's better to these?

  1. Just do require('.') (which will point to index.js)
  2. Remove the preinstall script
  3. Remove the node-read-yaml in devDependencies

@rishi-raj-jain
Copy link
Author

I noticed that the only time node-read-yaml is required is in here, in a test file that would never be run by a consumer:

const read = require('node-read-yaml');

That being the case, I think it's better to these?

  1. Just do require('.') (which will point to index.js)
  2. Remove the preinstall script
  3. Remove the node-read-yaml in devDependencies

But we can not remove the pre install script since it unlinks the package if it has the ability to

@imballinst
Copy link

But we can not remove the pre install script since it unlinks the package if it has the ability to

Maybe the repository owner has the better knowledge of it, but from my perspective, here's the flow:

  1. This package is referring its own in devDependencies here: https://github.com/dailyrandomphoto/node-read-yaml/blob/master/package.json#L22
  2. In order to prevent endless circular reference (I assume), the repository owner creates another package called unlink-self https://github.com/dailyrandomphoto/unlink-self, which purpose is only to delete node_modules/node-read-yaml.

If we just simply not have node-read-yaml in the devDependencies, that solves everything, right?

@rishi-raj-jain
Copy link
Author

Ah oh yes, you are right!

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.

Preinstall script fails on Windows under Powershell
2 participants