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 yarn when running inside winpty #634

Merged
merged 1 commit into from Jan 12, 2020
Merged

fix yarn when running inside winpty #634

merged 1 commit into from Jan 12, 2020

Conversation

jetersen
Copy link
Contributor

@jetersen jetersen commented Jan 10, 2020

@jetersen
Copy link
Contributor Author

This issue won't exist in https://github.com/yarnpkg/berry

This is only related to yarn v1

@jetersen
Copy link
Contributor Author

jetersen commented Jan 10, 2020

Not sure if we should add some sort of conditional logic to the getScript's render function based on version of yarn.

I also question the need for specifically running with yarn. Perhaps the better suggestion is to assume npx?

@arcanis
Copy link
Contributor

arcanis commented Jan 10, 2020

I also question the need for specifically running with yarn.

No, you must use yarn run if the packages are installed with Yarn. Npx has no idea about the mechanisms we use to layout the files, which are bound to differ.

Not sure if we should add some sort of conditional logic to the getScript's render function based on version of yarn.

At the very least I think you should add a check for Yarn 1 vs Yarn 2 (but only when winpty is an available command), as --no-interactive isn't a valid option in Yarn 2 (and it doesn't silently accept invalid options anymore).

@jetersen
Copy link
Contributor Author

As I suspected @arcanis :)

@jetersen
Copy link
Contributor Author

jetersen commented Jan 10, 2020

Since yarn v2 is the only one with pnp?

would it make sense for yarn v1 to use npx?
or do you want us to do the winpty dance?

@arcanis
Copy link
Contributor

arcanis commented Jan 10, 2020

I wouldn't recommend using npx for Yarn. I don't control this package and I can't ensure that it'll stay compatible with us. For all we know maybe npm will one day want to make their own install processes that will be incompatible with node_modules. It also looks fairly unmaintained so ... 🤷‍♀️

@jetersen
Copy link
Contributor Author

I reverted lots of the changes and made it use a simpler approch

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.

[4.0.0 | Git Bash | Windows 10] stdin is not a tty
3 participants