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

Allow disabling of console clearing #2495

Closed
hyperknot opened this issue Jun 7, 2017 · 39 comments
Closed

Allow disabling of console clearing #2495

hyperknot opened this issue Jun 7, 2017 · 39 comments

Comments

@hyperknot
Copy link

hyperknot commented Jun 7, 2017

When debugging an ejected script via console.logging different things, I found it quite difficult / annoying that multiple pieces of this script clear the the console when it detects stdout.isTTY (start.js and react-dev-utils/WebpackDevServerUtils.js).

I would like to recommend some kind of argument or environment variable to disable that behaviour, so that we wouldn't need to eject and modify and clone to simply enable console logging.

@viankakrisna
Copy link
Contributor

a detection of env variable (maybe REACT_APP_NO_CLEAR_CONSOLE) in here should do it https://github.com/facebookincubator/create-react-app/blob/master/packages/react-dev-utils/clearConsole.js#L13-L15

@hyperknot
Copy link
Author

@viankakrisna yes, that's a good idea as well!

@Timer
Copy link
Contributor

Timer commented Jun 18, 2017

I'm not sure this is going to be something we allow; why not edit the file in node_modules/ while debugging?

And you really shouldn't be using console debugging anyway 😅. Why not use something like VSCode with built-in debug support?

@viankakrisna
Copy link
Contributor

@Timer is there any more alternative? I've been doing console.log debugging in node since forever and don't want to switch editor :D

@Timer
Copy link
Contributor

Timer commented Jun 18, 2017

You can boot up with node debug or node --inspect and then open chrome to the provided URL and use devtools.

@viankakrisna
Copy link
Contributor

@Timer thanks! That's new to me. So that's how people inspect a nested objects in node huh? I thought everyone uses their terminal for debugging node code.... However, I've tried it in CRA repo but the instruction is quickly cleared for --inspect

"scripts": {
    "build": "node --inspect packages/react-scripts/scripts/build.js",
    "changelog": "lerna-changelog",
    "create-react-app": "tasks/cra.sh",
    "e2e": "tasks/e2e-simple.sh",
    "postinstall": "lerna bootstrap && cd packages/react-error-overlay/ && npm run build:prod",
    "publish": "tasks/release.sh",
    "start": "node --inspect packages/react-scripts/scripts/start.js",
    "test": "node --inspect packages/react-scripts/scripts/test.js --env=jsdom",
    "format": "prettier --trailing-comma es5 --single-quote --write 'packages/*/*.js' 'packages/*/!(node_modules)/**/*.js'",
    "precommit": "lint-staged"
  },

using node debug is not practical because it executes the script line by line... So i think configuring console clearing behavior could be added? 😄 or detect whether the current script is started using debug / --inspect.

I will happily add PR for contributing.md (or the root package.json) for this if we can also improve Contributor's Experience (CX?) :)

@michaek
Copy link

michaek commented Jul 27, 2017

I think this is worth considering, as the current console experience seems very strange to me. Clearing the console is kind of nice aesthetically (though not that important, in my opinion), but currently all output history in the console session is removed after running start which seems like something you would never actually want.

@SeanCannon
Copy link

The REACT_APP_NO_CLEAR_CONSOLE env var is a great idea.

@viankakrisna
Copy link
Contributor

or run it with FORCE_COLOR=true npm start | cat or FORCE_COLOR=true yarn start | cat if you use yarn. Taken from https://github.com/reasonml-community/reason-scripts#help-tips-and-tricks

@ghost
Copy link

ghost commented Apr 10, 2018

Please can we add REACT_APP_NO_CLEAR_CONSOLE env var. Using Windows, OSX, Linux is a nightmare!

@ghost
Copy link

ghost commented Apr 10, 2018

@viankakrisna and on Windows?

@viankakrisna
Copy link
Contributor

@johnunclesam not sure 😅 does the command works under git bash? i agree it's nicer to have env variable to achieve this functionality though

@patrickmcelwee
Copy link

This workaround from @viankakrisna breaks the ability to interact with the server from the console. For example, it removes the ability to detect if something is already running on the default port and suggest an available port. An environment variable to disable console clearing still has merit.

@Suor
Copy link

Suor commented Jun 28, 2018

I see no benefits to clearing console, and even consider this as harmful behavior. This should not stay the default, so no flag won't fix the issue.

Any deletion of user data should be intended not a side effect like here.

@lmorchard
Copy link

Not to dogpile here, but since some of the recommendations for customizing the build of a create-react-app without ejecting involve running separate build processes, it would be really handy if this thing didn't clear the terminal.

For example, I'm currently running Sass and CRA together in the same shell with npm-run-all --parallel and CRA is a rather rude roommate there. I was also thinking of running some linting and testing with other tools (e.g. sass-lint and mocha), but the output of those commands would also be lost with terminal clearing.

@corysimmons
Copy link

corysimmons commented Aug 20, 2018

^ this. cra blasts everything else npm-run-all might be doing.. Please just add a flag like --no-console-clear to cra cli. People can tweak their package.json start script with it.

This hack does work.

@hyperknot
Copy link
Author

Yes, npm-run-all and concurrently suffers greatly from this behaviour. If so many variables are supported in .env, can there be a simple boolean for this?

@mmartinson
Copy link

This is a painful one for me too. If we want to favour interactivity over configuration as per the core ideas in the contributing guidelines, how about we add a prompt to clear the console instead of automatically clearing it?

This obviously is weird, because a prompt to clear the terminal is practically useless with how easily it is manually, but it fits the requirements without deleting the debug data if the maintainers aren't willing to compromise on this one with a flag.

I find the tradeoff being made here currently leans towards aesthetics over usefulness, and in this case is obviously disempowering over helpful for a good number of people.

@wmira
Copy link

wmira commented Oct 17, 2018

this should be allowed. e.g. I was trying to figure out why proxy configuration wasn't being used ( which was a bug on my CRA version) and the console clearing making it hard for me to see what is going on. A simple flag/env can be passed for this

@alistair-hmh
Copy link

alistair-hmh commented Nov 8, 2018

or run it with FORCE_COLOR=true npm start | cat or FORCE_COLOR=true yarn start | cat if you use yarn. Taken from reasonml-community/reason-scripts#help-tips-and-tricks

@viankakrisna - How can I do this from a Node process spawn?

Here's what I'm working with...

const reactProcess = spawn('npx', ['react-scripts', 'start'])

@alistair-hmh
Copy link

alistair-hmh commented Nov 8, 2018

This works...

// scripts/launch-app.js
const {spawn} = require('child_process');

const reactProcess = spawn('npx', ['react-scripts', 'start'], {
    env: Object.assign(process.env, {FORCE_COLOR: true})
    detatched: true
});

reactProcess.unref();

const removeCliClearCodes = text => {
	return text.replace(/\\033\[2J/g, '');
}

reactProcess.stdout.on('data', data => {
    const text = String(data);
    const output = removeCliClearCodes(text);
	console.log(text);
});
// package.json
{
	"scripts": {
		"start": "scripts/launch-app"
	}
}

@acusti
Copy link

acusti commented Nov 28, 2018

@alistair-hmh Thanks for posting that workaround! I noticed that in my use case, the 3rd options argument to spawn is unnecessary. I realized that fact when I noticed in your snippet that detached was misspelled as detatched, and then noticed that adding the FORCE_COLOR env variable didn’t do anything for me. In my case, listening to stdout and console.logging the result of removeCliClearCodes did the trick and was the only necessary piece.

In the end, I actually wound up taking a different approach where I replaced the react-dev-utils/clearConsole module entirely with a noop, which ensured that I still get the proper color coding in my console. It involved some pretty nauseating hackery. In my scripts/start.js file (from an ejected create-react-app project), I replaced (https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/scripts/start.js#L40-L45):

const {
  choosePort,
  createCompiler,
  prepareProxy,
  prepareUrls,
} = require('react-dev-utils/WebpackDevServerUtils');

with:

// Replace react-dev-utils/clearConsole with a noop in WebpackDevServerUtils
// to prevent it from clearing the console on every compile
const noop = () => {};
const webpackDevServerUtilsPath = require.resolve('react-dev-utils/WebpackDevServerUtils');
const resolveModule = (name) =>
    name.charAt(0) !== '.' ? name : path.resolve(path.dirname(webpackDevServerUtilsPath), name);

const consolePreserveExports = {};
const consolePreserveContext = {
    require: (name) => (name.endsWith('clearConsole') ? noop : require(resolveModule(name))),
    console,
    exports: consolePreserveExports,
    module: {
        exports: consolePreserveExports,
    },
    process,
};

vm.runInNewContext(fs.readFileSync(webpackDevServerUtilsPath), consolePreserveContext);
const {
    choosePort,
    createCompiler,
    prepareProxy,
    prepareUrls,
} = consolePreserveContext.module.exports;

🤢

@russellr922
Copy link

@acusti thank you for the solution. For those maybe not so familiar with node you will need to also add the path and vm deps before this code for this to work:

const vm = require('vm'); const path = require('path');

@Timer are you guys still not allowing this?

@stale
Copy link

stale bot commented Jan 6, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 6, 2019
@lmorchard
Copy link

Not to be snarky, but closing via stale bot seems like a rude ending to this issue

@mmartinson
Copy link

Ya, I'm curious to see a resolution on this. Is the decision that maintainers want to leave this as is? Would a PR be accepted? What would the ergonomics of an accepted solution look like?

@drewswaycool
Copy link

drewswaycool commented Jan 8, 2019

I've created this PR as a possible solution to the issue.

#6102

@melMass
Copy link

melMass commented Jan 21, 2019

@drewswaycool Thanks I think the PR needs to add an option and not remove the default behaviour that some may rely on. I'm not familiar with CRA's code but you should add a flag or env option.

It's the first time I use a CLI tool that clears the current shell history (most actually scroll to the bottom keeping the buffer intact), maybe that could be the best of both worlds?

@amiramitai
Copy link

amiramitai commented Jan 24, 2019

Quick and dirty solution.

Change this in start.js from:

const WebpackDevServer = require('webpack-dev-server');
const clearConsole = require('react-dev-utils/clearConsole');

to:

require('react-dev-utils/clearConsole');
const clearConsole = ()=>{}
require.cache[require.resolve('react-dev-utils/clearConsole')].exports = clearConsole
const WebpackDevServer = require('webpack-dev-server');

@stale
Copy link

stale bot commented Feb 23, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Feb 23, 2019
@drewswaycool
Copy link

I don't believe this issue should be stale just as #6102 is not stale.

@stale stale bot removed the stale label Feb 26, 2019
@drewswaycool
Copy link

drewswaycool commented Mar 12, 2019

Update from @Timer here --> #6102 (review)

This is done intentionally. We'll likely not merge this.

👎

@codepunkt
Copy link

@gaearon @Timer There are literally tons of people working on projects based on create-react-app. A lot of them favor not ejecting so they can keep updating react-scripts, which is a sensible thing to do.

Still, there are dozens of reasons why adjust the build process might be worthwhile, so people tend to use create-react-app with tools such as react-app-rewired or rescripts to alter the configuration without ejecting or in combination with concurrently or npm-run-all to run things in parallel.

In all of these important and comprehensible use cases, the clearing of the console by create-react-app massively hampers development and interferes with the ability to simply get stuff done.

It is understandable that PR #6102 is not merged because the team has decided that they want to keep clearing the console despite the obvious drawbacks in a lot of cases such as the ones detailed above. Still, this issue persists and keeps being a problem to a lot of people.

Please reconsider the introduction of an additional environment variable that allows users to prevent the console clearing from happening. This way, we can default to clearing but allow users with special use cases to prevent it in a sensible way.

@bugzpodder
Copy link

bugzpodder commented May 1, 2019

@drewswaycool @codepunkt @lmorchard @acusti @razzfox How do you feel about using concurrently as a workaround for this issue:

yarn add concurrently
yarn concurrently npm:start

Or

npx concurrently npm:start

Or

FORCE_COLOR=true yarn start | cat

Or if you have lerna installed in a monorepo,

lerna run --parallel start

It will break input interactivity but it seems these two workarounds should be sufficient for most use cases?

@russellr922
Copy link

russellr922 commented May 2, 2019

@bugzpodder Clearing the console is intrusive, leaving it alone is not.... why is being intrusive the default behavior here. This makes no sense.

@drewswaycool
Copy link

@bugzpodder I think the bummer here is the idea of needing a workaround at all.. It just feels like out of the box clearing the console should not be the default. Workarounds are fine.. but no workarounds are better.

@bugzpodder
Copy link

Given that this issue is about providing a workaround to optionally not clear the console and several ways were provided to do so, I think we can finally resolve this issue. I don't see the need to further continuing this debate given the opinionated nature of CRA's defaults.

@russellr922

This comment has been minimized.

@lmorchard
Copy link

Typically, "workarounds" are things you do to avoid a bug while it's being fixed - not generally advised practice.

@lock lock bot locked and limited conversation to collaborators May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests