gulpjs / gulp-cli

Command Line Interface for gulp.
MIT License
401 stars 106 forks source link

Show error info for --require param problem #176

Closed smialy closed 5 years ago

smialy commented 5 years ago

I spent some time to figure out why: gulp --require @babel/register ... not works. Was missing @babel/core :]

phated commented 5 years ago

Can you update the tests and check why appveyor is failing?

smialy commented 5 years ago

I've seen and check in my local machine. All tests pass also on TravisCI. Only on Appveyor not and it's looks that something is broken there.

sttk commented 5 years ago

I met the same error recently. This cause seems that timestamps in log outputs were not erased because of ANSI color escape sequences.

I don't know yet why these escape sequence are outputted in testing. (Needed explicit --no-color flag?)

phated commented 5 years ago

@sttk would this change I made in fancy-log cause this new failure? https://github.com/gulpjs/fancy-log/commit/9a3a6620434ed3eca8fbd6cf0683410ade38c6d4

smialy commented 5 years ago

Nice color console require one extra condition: if(testing) { disableColors() } :]

phated commented 5 years ago

Interesting. I'm going to have @sttk chime in as well because I'm guessing I broke something within fancy-log.

smialy commented 5 years ago

Some plan or idea how to fix it?

sttk commented 5 years ago

@phated The issue you fixed in above link might be similar to #163. If so, this is not Windows' issue but terminal's (AppVeyor uses MinGW?) or color-support's issue. But I have no good idea to solve this problem.

Sample code: (color-test.js)

#!/usr/bin/env node
var colorSupport = require('color-support');
console.log('Color support: ', colorSupport());

On MinGW:

$ node color-test.js
Color support:  { level: 1, hasBasic: true, has256: false, has16m: false }

$ node.exe color-test.js
Color support:  false

$ ./color-test.js
Color support:  false

On Command Prompt:

C:\>node.exe color-test.js
Color support:  { level: 1, hasBasic: true, has256: false, has16m: false }

C:\>node.exe color-test.js
Color support:  { level: 1, hasBasic: true, has256: false, has16m: false }
sttk commented 5 years ago

@smialy @phated I'll attach --no-color flag to gulp command automatically in gulp-test-tool. At least, it will solve this problem about gulp-cli tests.

sttk commented 5 years ago

I've published gulp-test-tools v0.6.2. Retry test on AppVeyor.

smialy commented 5 years ago

Who can do it or how?

sttk commented 5 years ago

Restart link is not displayed on my account.

@phated Can you restart appveyor test?

smialy commented 5 years ago

Merge ready?

phated commented 5 years ago

@sttk how can we test that errors are being shown correctly? I didn't even realize this method had a 2nd argument.

sttk commented 5 years ago

@phated By this line, an error object is passed as 2nd argument.

@smialy Can you write a test for this modification in test/flag-require.js?

sttk commented 5 years ago

@phated Are you ok with this?

phated commented 5 years ago

Should we do some better formatting on this? I'm thinking something like:

Failed to load external module @babel/register
    Encountered error: [Error: Missing @babel/core]

Thoughts?

sttk commented 5 years ago

@smialy What about the format @phated suggeted? Could you modify like this?

smialy commented 5 years ago

In this way?

[09:16:57] Failed to load external module @babel/register
    Error: Cannot find module '@babel/core'
.../gulpfile.js:1
SyntaxError: Unexpected token export
sttk commented 5 years ago

I think it's enough only the first two lines and it's good except that.

smialy commented 5 years ago

Some problem with integration travis-ci and github?

sttk commented 5 years ago

@smialy Your code is no problem. I couldn't restart AppVeyor tests, but I merged your code in my repository, runned it on appveyor, and it passed.

@phated How is this change?

phated commented 5 years ago

@sttk sorry that I haven't been around to review, I started a new job a couple weeks ago. Are you satisfied with the PR? I'll restart the appveyor build but I'm confused by what dependency won't install.

sttk commented 5 years ago

The cause of Appveyor's error is bump deps of normalize-package-data : is-builtin-module "^1.0.0"=>"^3.0.0". (That was made on Jan. 29).

These modules are used in yargs, and is-builtin-module v3.0.0 targets Node >=v6.0. Since this bumping of normalize-package-data is made as a patch (v2.4.0=>v2.4.1), this error cannot be avoided on the older versions of Node.

sttk commented 5 years ago

@phated Please restart Appveyor's test because normalize-package-data' was changed back to use is-builtin-module^1.0.0. The test will succeed.

phated commented 5 years ago

Why was this closed?

phated commented 5 years ago

@sttk should I publish this version or do you want to land some more stuff first?

phated commented 5 years ago

@smialy thanks for all the hard work on this!!!

sttk commented 5 years ago

@phated I think it's better to publish liftoff with merging the pr js-liftoff#94 before gulp-cli because requireFail messages will increase by this pr.

phated commented 5 years ago

I meant to follow up on this when 2.1.0 was published. It's been published for 11 days so this fix is available now. Thanks again!