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

Fixes potential freeze on win+node10 interactive upgrades (#5949) #6635

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

pfeigl
Copy link
Contributor

@pfeigl pfeigl commented Nov 5, 2018

There is a potential freeze when running interactive upgrades after selecting the options. (See #6592, #6367, #6198, #5949)

This boiles down to a problem in the inquirer library which is used internally to provide the possible options on the CLI. Updating inquirer to the latest version fixes the problem.

All tests keep passing also with the new version of inquirer.

There is a potential freeze when running interactive upgrades after selecting the options.
This boiles down to a problem in the `inquirer` library which is used internally to provide the possible options.
Updating `inquirer`to the latest version fixes the problem.
@pfeigl pfeigl force-pushed the fix_win_node10_freeze branch from 43a23b4 to 148048d Compare November 5, 2018 14:24
@buildsize
Copy link

buildsize bot commented Nov 5, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.09 MB 1.1 MB 10.52 KB (1%)
yarn-[version].js 4.27 MB 4.46 MB 192.91 KB (4%)
yarn-legacy-[version].js 4.45 MB 4.63 MB 192.94 KB (4%)
yarn-v[version].tar.gz 1.1 MB 1.11 MB 14.46 KB (1%)
yarn_[version]all.deb 804.98 KB 812.6 KB 7.62 KB (1%)

@s-bauer
Copy link

s-bauer commented Nov 5, 2018

Awesome! Let's hope this gets approved fast!

@arcanis
Copy link
Member

arcanis commented Nov 5, 2018

The problem I see is that the enquirer part of Yarn isn't well tested, to the best of my knowledge (since it's quite harder to test user inputs etc - not impossible, but harder).

Can you confirm that a thorough use of upgrade-interactive with this patch enabled doesn't have any side effect? Can you also check yarn version, and yarn add lodash@does-not-exist?

@pfeigl
Copy link
Contributor Author

pfeigl commented Nov 5, 2018

I ran quite some interactive upgrades on existing projects with my local build (major and minor package upgrades) which all worked as expected.

Additionally tested your two additional commands which also worked as expected. Also selecting from the list of the available lodash versions worked as expected.

All those tests have been run on my local devbox (win10, node10) and therefor not on any linux/mac box.

@arcanis
Copy link
Member

arcanis commented Nov 5, 2018

Awesome, thank you 😃

@arcanis arcanis merged commit 5539fa2 into yarnpkg:master Nov 5, 2018
@pfeigl
Copy link
Contributor Author

pfeigl commented Nov 5, 2018

Nice, thanks for the fast merge! Btw reg. the Changelog, the library used is inquirer (https://github.com/sboudrias/Inquirer.js) not enquirer (http://enquirer.io/), which pretty much do the same but are different implementations.

@arcanis
Copy link
Member

arcanis commented Nov 5, 2018

Woops, fixed 👍

@pfeigl pfeigl deleted the fix_win_node10_freeze branch November 5, 2018 17:22
@rally25rs
Copy link
Contributor

rally25rs commented Nov 5, 2018

Thanks for the contribution @pfeigl ! It's going to make a lot of Windows users happy. 👍

I know it's already merged, but did anyone test this on Node v4 to make sure we don't break that compatibility?

@pfeigl
Copy link
Contributor Author

pfeigl commented Nov 6, 2018

Unfortunately I did not and looking at older release notes of inquirer, they seem to have droped support for Node v4, see here: https://github.com/SBoudrias/Inquirer.js/releases/tag/v4.0.0

I'm not sure whether they mean you cannot build inquirer with node4 or whether consuming the library is also limited to node v5+

They even state this on their main page:

Version 4.x only supports Node 6 and over. For Node 4 support please use version 3.x.

Unfortunately they don't seem to backport their fixes into older versions, as the latest 3.x is 3.3.0 which is what yarn used previously

@arcanis
Copy link
Member

arcanis commented Nov 6, 2018

Compat is fixed in #6640 - I just configured webpack to transpile inquirer down to ES5. It seems like there are no runtime dependencies on Node 6+, so that should hold for now.

We really need to drop Node 4 😕

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 22, 2018
Changelog tracks back up to 1.12.0 only.

## 1.12.3

**Important:** This release contains a cache bump. It will cause the very first install following the upgrade to take slightly more time, especially if you don't use the [Offline Mirror](https://yarnpkg.com/blog/2016/11/24/offline-mirror/) feature. After that everything will be back to normal.

- Fixes an issue with `yarn audit` when using workspaces

  [6625](yarnpkg/yarn#6639) - [**Jeff Valore**](https://twitter.com/codingwithspike)

- Uses `NODE_OPTIONS` to instruct Node to load the PnP hook, instead of raw CLI arguments

  **Caveat:** This change might cause issues for PnP users having a space inside their cwd (cf [nodejs/node24065](nodejs/node#24065))

  [6479](yarnpkg/yarn#6629) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes Gulp when used with Plug'n'Play

  [6623](yarnpkg/yarn#6623) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an issue with `yarn audit` when the root package was missing a name

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with `yarn audit` when a package was depending on an empty range

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with how symlinks are setup into the cache on Windows

  [6621](yarnpkg/yarn#6621) - [**Yoad Snapir**](https://github.com/yoadsn)

- Upgrades `inquirer`, fixing `upgrade-interactive` for users using both Node 10 and Windows

  [6635](yarnpkg/yarn#6635) - [**Philipp Feigl**](https://github.com/pfeigl)

- Exposes the path to the PnP file using `require.resolve('pnpapi')`

  [6643](yarnpkg/yarn#6643) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.2

This release doesn't actually exists and was caused by a quirk in our systems.

## 1.12.1

- Ensures the engine check is ran before showing the UI for `upgrade-interactive`

  [6536](yarnpkg/yarn#6536) - [**Orta Therox**](https://github.com/orta)

- Restores Node v4 support by downgrading `cli-table3`

  [6535](yarnpkg/yarn#6535) - [**Mark Stacey**](https://github.com/Gudahtt)

- Prevents infinite loop when parsing corrupted lockfiles with unterminated strings

  [4965](yarnpkg/yarn#4965) - [**Ryan Hendrickson**](https://github.com/rhendric)

- Environment variables now have to **start** with `YARN_` (instead of just contain it) to be considered

  [6518](yarnpkg/yarn#6518) - [**Michael Gmelin**](https://blog.grem.de)

- Fixes the `extensions` option when used by `resolveRequest`

  [6479](yarnpkg/yarn#6479) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes handling of empty string entries for `bin` in package.json

  [6515](yarnpkg/yarn#6515) - [**Ryan Burrows**](https://github.com/rhburrows)

- Adds support for basic auth for registries with paths, such as artifactory

  [5322](yarnpkg/yarn#5322) - [**Karolis Narkevicius**](https://twitter.com/KidkArolis)

- Adds 2FA (Two Factor Authentication) support to publish & alike

  [6555](yarnpkg/yarn#6555) - [**Krzysztof Zbudniewek**](https://github.com/neonowy)

- Fixes how the `files` property is interpreted to bring it in line with npm

  [6562](yarnpkg/yarn#6562) - [**Bertrand Marron**](https://github.com/tusbar)

- Fixes Yarn invocations on Darwin when the `yarn` binary was symlinked

  [6568](yarnpkg/yarn#6568) - [**Hidde Boomsma**](https://github.com/hboomsma)

- Fixes `require.resolve` when used together with the `paths` option

  [6565](yarnpkg/yarn#6565) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.0

- Adds initial support for PnP on Windows

  [6447](yarnpkg/yarn#6447) - [**John-David Dalton**](https://twitter.com/jdalton)

- Adds `yarn audit` (and the `--audit` flag for all installs)

  [6409](yarnpkg/yarn#6409) - [**Jeff Valore**](https://github.com/rally25rs)

- Adds a special logic to PnP for ESLint compatibility (temporary, until [eslint/eslint10125](eslint/eslint#10125) is fixed)

  [6449](yarnpkg/yarn#6449) - [**Maël Nison**](https://twitter.com/arcanis)

- Makes the PnP hook inject a `process.versions.pnp` variable when setup (equals to `VERSIONS.std`)

  [6464](yarnpkg/yarn#6464) - [**Maël Nison**](https://twitter.com/arcanis)

- Disables by default (configurable) the automatic migration of the `integrity` field. **It will be re-enabled in 2.0.**

  [6465](yarnpkg/yarn#6465) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes the display name of the faulty package when the NPM registry returns corrupted data

  [6455](yarnpkg/yarn#6455) - [**Grey Baker**](https://github.com/greysteil)

- Prevents crashes when running `yarn outdated` and the NPM registry forgets to return the `latest` tag

  [6454](yarnpkg/yarn#6454) - [**mad-mike**](https://github.com/mad-mike)

- Fixes `yarn run` when used together with workspaces and PnP

  [6444](yarnpkg/yarn#6444) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an edge case when peer dependencies were resolved multiple levels deep (`webpack-dev-server`)

  [6443](yarnpkg/yarn#6443) - [**Maël Nison**](https://twitter.com/arcanis)
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 participants