Show error info for --require param problem #176

smialy · 2018-12-11T10:54:20Z

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

phated · 2018-12-11T19:28:48Z

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

smialy · 2018-12-12T07:53:21Z

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 · 2018-12-12T14:27:16Z

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 · 2018-12-12T18:30:12Z

@sttk would this change I made in fancy-log cause this new failure? gulpjs/fancy-log@9a3a662

smialy · 2018-12-12T23:31:01Z

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

phated · 2018-12-12T23:39:30Z

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

smialy · 2018-12-13T19:46:27Z

Some plan or idea how to fix it?

sttk · 2018-12-14T12:17:33Z

@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 · 2018-12-14T12:18:31Z

@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 · 2018-12-14T15:33:34Z

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

smialy · 2018-12-14T15:42:53Z

Who can do it or how?

sttk · 2018-12-14T16:01:47Z

Restart link is not displayed on my account.

@phated Can you restart appveyor test?

smialy · 2018-12-14T18:25:45Z

Merge ready?

phated · 2018-12-16T21:50:40Z

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

sttk · 2018-12-18T14:41:21Z

@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 · 2018-12-18T14:43:14Z

index.js

-cli.on('requireFail', function(name) {
- log.warn(ansi.yellow('Failed to load external module'), ansi.magenta(name));
+cli.on('requireFail', function(name, error) {
+ var message = name + (error ? ' [' + error + ']' : '');


Since error is an error object, this code outputs too many lines including stack trace. Isn't it enough error message? And I think it's better that the error message is outputted to next line.

Fo me this message:

Failed to load external module @babel/register [Error: Cannot find module '@babel/core']

is more helpful than:

Failed to load external module @babel/register

I have no idea why @babel/register doesn't install as dependency @babel/core but ending was that I add console.log(...) in this place to find this problem :)

@smialy Sorry, I mistake console.log(new Error() + '') for console.log(new Error()).
The former does not output stack trace.

@smialy Now I think this modification is good, but add/modify a test case to test/flag-require.js, please.

Add small unittest checking extra error message for --require option in case that app has some problem to include it.

sttk

I think this modification is good and has no problem.

sttk · 2018-12-21T14:02:08Z

@phated Are you ok with this?

phated · 2018-12-21T21:00:16Z

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 · 2018-12-25T12:40:03Z

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

smialy · 2019-01-14T08:19:33Z

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 · 2019-01-14T12:58:09Z

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

smialy · 2019-01-16T07:10:47Z

Some problem with integration travis-ci and github?

sttk

I've finished review and please change.

index.js

sttk · 2019-01-20T10:50:39Z

index.js

+cli.on('requireFail', function(name, error) {
+ log.warn(
+ ansi.yellow('Failed to load external module'),
+ ansi.yellow(name)


Sorry, my explanation might be bad. Please modify only a module name to magenta as is.

sttk · 2019-01-29T15:16:18Z

@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 · 2019-01-30T03:07:41Z

@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 · 2019-01-30T14:32:44Z

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 · 2019-02-02T07:08:17Z

@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 · 2019-02-15T23:03:14Z

Why was this closed?

phated · 2019-02-23T05:42:49Z

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

phated · 2019-02-23T05:43:04Z

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

sttk · 2019-02-23T06:10:27Z

@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 · 2019-04-06T00:54:32Z

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!

Show error info for --require param problem

287140f

sttk reviewed Dec 18, 2018

View reviewed changes

Unittest for extra error message in --require option.

ad9fc9a

Add small unittest checking extra error message for --require option in case that app has some problem to include it.

sttk approved these changes Dec 21, 2018

View reviewed changes

sttk suggested changes Jan 16, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

sttk reviewed Jan 20, 2019

View reviewed changes

Update format in --require extend error

c6b9ce3

smialy closed this Feb 13, 2019

phated reopened this Feb 15, 2019

sttk approved these changes Feb 16, 2019

View reviewed changes

phated merged commit 7d9d1a0 into gulpjs:master Feb 23, 2019

Show error info for --require param problem #176

Show error info for --require param problem #176

Conversation

smialy commented Dec 11, 2018 • edited Loading

phated commented Dec 11, 2018

smialy commented Dec 12, 2018 • edited Loading

sttk commented Dec 12, 2018

phated commented Dec 12, 2018

smialy commented Dec 12, 2018 • edited Loading

phated commented Dec 12, 2018

smialy commented Dec 13, 2018

sttk commented Dec 14, 2018

sttk commented Dec 14, 2018

sttk commented Dec 14, 2018

smialy commented Dec 14, 2018 • edited Loading

sttk commented Dec 14, 2018

smialy commented Dec 14, 2018 • edited Loading

phated commented Dec 16, 2018

sttk commented Dec 18, 2018

sttk Dec 18, 2018

Choose a reason for hiding this comment

smialy Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

sttk Dec 18, 2018

Choose a reason for hiding this comment

sttk Dec 19, 2018

Choose a reason for hiding this comment

sttk left a comment

Choose a reason for hiding this comment

sttk commented Dec 21, 2018

phated commented Dec 21, 2018

sttk commented Dec 25, 2018

smialy commented Jan 14, 2019 • edited Loading

sttk commented Jan 14, 2019

smialy commented Jan 16, 2019

sttk left a comment

Choose a reason for hiding this comment

sttk Jan 20, 2019

Choose a reason for hiding this comment

sttk commented Jan 29, 2019

phated commented Jan 30, 2019

sttk commented Jan 30, 2019

sttk commented Feb 2, 2019

phated commented Feb 15, 2019

phated commented Feb 23, 2019

phated commented Feb 23, 2019

sttk commented Feb 23, 2019

phated commented Apr 6, 2019

smialy commented Dec 11, 2018 •

edited

Loading

smialy commented Dec 12, 2018 •

edited

Loading

smialy commented Dec 12, 2018 •

edited

Loading

smialy commented Dec 14, 2018 •

edited

Loading

smialy commented Dec 14, 2018 •

edited

Loading

smialy Dec 18, 2018 •

edited

Loading

smialy commented Jan 14, 2019 •

edited

Loading