feat(cli): Disable only header/footer in nested commands, not all output #4811

sth · 2017-10-31T03:06:11Z

Summary

Fixes #4615. Disabling all Yarn output in nested commands with YARN_SILENT is a bit much, we usually want to see the output. This pull request introduces a new environment variable YARN_WRAP_OUTPUT that can be set to 0 to disable the header and footer Yarn normally displays.

Disabling the header/footer might also be useful in other situations, like other tools calling Yarn, so the YARN_WRAP_OUTPUT variable has general use.

Test plan

Existing integration tests.

This is done instead of disabling all output, which was a bit too much (see issue #4615).

BYK

Very simple and elegant solution, I love it.

Added a comment to make the detection and code a bit better. Will merge after you address that.

Thanks!

BYK · 2017-10-31T09:29:31Z

src/cli/index.js

@@ -207,7 +207,8 @@ export function main({
  reporter.initPeakMemoryCounter();

  const config = new Config(reporter);
-  const outputWrapper = !commander.json && command.hasWrapper(commander, commander.args);
+  const outputWrapperEnv = process.env.YARN_WRAPOUTPUT === undefined || process.env.YARN_WRAPOUTPUT == '1';


I'd go with the following:

const disableOutputWrapper = process.env.YARN_WRAPOUTPUT === '0' || process.env.YARN_WRAPOUTPUT === 'false'; const shouldWrapOutput = !disableOutputWrapper && !commander.json && command.hasWrapper(commander, commander.args); if (shouldWrapOutput) {

buildsize · 2017-10-31T16:54:46Z

This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 448 bytes (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	859.9 KB	859.91 KB	13 bytes (0%)
yarn-[version].js	3.78 MB	3.78 MB	272 bytes (0%)
yarn-legacy-[version].js	3.83 MB	3.83 MB	272 bytes (0%)
yarn-v[version].tar.gz	865.5 KB	865.55 KB	47 bytes (0%)
yarn_[version]all.deb	654.58 KB	654.42 KB	-156 bytes (0%)

sth · 2017-10-31T17:00:54Z

I moved the boolification of the environment variable into a separate function. This function can then also be used for other variables, like YARN_SILENCE, to make sure they all accept the same values as true/false.

The functionality of boolifyWithDefault() could get integrated directly into boolify(), but it seemed clearer to me as a separate function.

BYK

Love the refactoring, thanks a lot!

BYK · 2017-11-01T11:06:57Z

src/cli/commands/run.js

-      // propagate YARN_SILENT env variable to executed commands
-      process.env.YARN_SILENT = '1';
+      // Disable wrapper in executed commands
+      process.env.YARN_WRAP_OUTPUT = 'false';


Note to self: do not mutate process.env in any way.

edmorley · 2017-11-01T11:26:54Z

Many thanks for this! :-)

**Summary** Use `boolifyWithDefault()` to determine if environment variable values are `true` or `false`. This ensures that all environment variables interpret the same values the same way. This changes the behavior of `YARN_SILENT` and `YARN_IGNORE_PATH` if they have "unexpected" values, all nonempty stings beside `"0"` and `"false"` are now interpreted as `true`. For example `YARN_SILENT=hello` was interpreted as `false` before, now it is `true`. This makes some sense since those values are truthy in Javascript, but generally makes things more predictable if it works like that for all yarn environment variables. `YARN_SILENT=true` was also interpreted as `false`. This now definitely makes more sense since it will be interpreted as `true`. See also [#4811](#4811 (comment)). **Test plan** There should be no change to the existing intended functionality and the existing tests still pass.

**Summary** Tests started failing on Travis after #4811, somewhat randomly, due to the mexpecting the unwrapped output. This PR fixes those expectations and moves normalize-manifest tests to snapshots since that's easier than updating 40+ JSON files by hand. **Test plan** Tests should pass on all platforms and CI and locally.

**Summary** Tests started failing on Travis after #4811, somewhat randomly, due to them expecting the unwrapped output. This PR fixes those expectations and moves normalize-manifest tests to snapshots since that's easier than updating 40+ JSON files by hand. **Test plan** Tests should pass on all platforms and CI and locally.

…put (yarnpkg#4811) **Summary** Fixes yarnpkg#4615. Disabling all Yarn output in nested commands with `YARN_SILENT` is a bit much, we usually want to see the output. This pull request introduces a new environment variable `YARN_WRAP_OUTPUT` that can be set to `0` to disable the header and footer Yarn normally displays. Disabling the header/footer might also be useful in other situations, like other tools calling Yarn, so the `YARN_WRAP_OUTPUT` variable has general use. **Test plan** Existing integration tests.

…npkg#4823) **Summary** Use `boolifyWithDefault()` to determine if environment variable values are `true` or `false`. This ensures that all environment variables interpret the same values the same way. This changes the behavior of `YARN_SILENT` and `YARN_IGNORE_PATH` if they have "unexpected" values, all nonempty stings beside `"0"` and `"false"` are now interpreted as `true`. For example `YARN_SILENT=hello` was interpreted as `false` before, now it is `true`. This makes some sense since those values are truthy in Javascript, but generally makes things more predictable if it works like that for all yarn environment variables. `YARN_SILENT=true` was also interpreted as `false`. This now definitely makes more sense since it will be interpreted as `true`. See also [yarnpkg#4811](yarnpkg#4811 (comment)). **Test plan** There should be no change to the existing intended functionality and the existing tests still pass.

…pkg#4852) **Summary** Tests started failing on Travis after yarnpkg#4811, somewhat randomly, due to them expecting the unwrapped output. This PR fixes those expectations and moves normalize-manifest tests to snapshots since that's easier than updating 40+ JSON files by hand. **Test plan** Tests should pass on all platforms and CI and locally.

sth added 2 commits October 31, 2017 03:12

Only disable header/footer in nested commands

b02b37f

This is done instead of disabling all output, which was a bit too much (see issue #4615).

Use clearer environment variable name

99e64a3

BYK requested changes Oct 31, 2017

View reviewed changes

sth added 2 commits October 31, 2017 17:32

Improved variable names and boolification

fe54fb2

Renamed env variable to YARN_WRAP_OUTPUT

518cb7c

BYK approved these changes Nov 1, 2017

View reviewed changes

BYK changed the title ~~Disable only header/footer in nested commands, not all output.~~ feat(cli): Disable only header/footer in nested commands, not all output Nov 1, 2017

BYK merged commit 6cb226f into yarnpkg:master Nov 1, 2017

sth deleted the issue-4615-no-nested-wrapper branch November 1, 2017 14:32

sth mentioned this pull request Nov 1, 2017

fix(config): Use boolifyWithDefault() for bools from environment #4823

Merged

BYK mentioned this pull request Nov 3, 2017

test: Fix flaky tests after #4811 due to bad assumptions #4852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): Disable only header/footer in nested commands, not all output #4811

feat(cli): Disable only header/footer in nested commands, not all output #4811

sth commented Oct 31, 2017 •

edited by BYK

Loading

BYK left a comment

BYK Oct 31, 2017

buildsize bot commented Oct 31, 2017 •

edited

Loading

sth commented Oct 31, 2017 •

edited

Loading

BYK left a comment

BYK Nov 1, 2017

edmorley commented Nov 1, 2017

feat(cli): Disable only header/footer in nested commands, not all output #4811

feat(cli): Disable only header/footer in nested commands, not all output #4811

Conversation

sth commented Oct 31, 2017 • edited by BYK Loading

BYK left a comment

Choose a reason for hiding this comment

BYK Oct 31, 2017

Choose a reason for hiding this comment

buildsize bot commented Oct 31, 2017 • edited Loading

sth commented Oct 31, 2017 • edited Loading

BYK left a comment

Choose a reason for hiding this comment

BYK Nov 1, 2017

Choose a reason for hiding this comment

edmorley commented Nov 1, 2017

sth commented Oct 31, 2017 •

edited by BYK

Loading

buildsize bot commented Oct 31, 2017 •

edited

Loading

sth commented Oct 31, 2017 •

edited

Loading