sighjs / sigh

multi-process expressive build system for the web and node.js, built using baconjs observables
209 stars 12 forks source link

sigh process exits when trying to construct identity source map for unparseable javascript. #7

Closed PatrikLythell closed 9 years ago

PatrikLythell commented 9 years ago

What about error handling? The process is now killed if any plugins throws an error. Errors in how streams are handled should probably throw but for example illegal tokens in javascript should not kill the process with -w flag.

What are you thoughts @ohjames?

insidewhy commented 9 years ago

It doesn't work that way, here's what happens if I add a syntax error to one of sigh's source files, then remove that error:

james@lonchi sigh % sigh -w build
 * pipeline build-sources complete: 1.236 seconds
 * pipeline build-tests complete: 1.667 seconds
 ! error: pipeline build-sources
 ! Error: src/gulp-adapter.js: Unexpected token (9:6)
 * pipeline build-sources complete: 0.206 seconds

Could you paste the sigh file you are seeing this in?

insidewhy commented 9 years ago

Please re-open this issue after providing some context.

insidewhy commented 9 years ago

Maybe you saw this writing your own plugin? This could be because you need to handle errors in a way that Bacon can catch them. Bacon cannot catch errors thrown inside of streams, but it is trivial to do so if you use Bacon.fromPromise as then rejected promises are converted to bacon errors (and with A* promises errors thrown in a synchronous context reject the promise).

If you are not using a promise you need to return a Bacon.Error object from your stream handler.

PatrikLythell commented 9 years ago

Sorry I might have misunderstood something.

patriklythell:sigh-test patriklythell$ sigh -w

/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:2336
        var error = new Error('Line ' + line + ': ' + description);
                    ^
Error: Line 4: Unexpected token ILLEGAL
    at createError (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:2336:21)
    at unexpectedTokenError (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:2412:13)
    at throwUnexpectedToken (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:2416:15)
    at scanStringLiteral (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:1102:13)
    at advance (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:1565:20)
    at collectToken (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:1605:17)
    at lex (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:1647:61)
    at Object.tokenize (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/esprima/esprima.js:5369:21)
    at generateIdentitySourceMap (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/sigh-core/src/sourceMap.js:64:26)
    at _default._createClass.get (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/src/Event.js:2:60)
    at _default.writeEvent (/Users/patriklythell/Sites/sigh-test/node_modules/sigh/lib/plugin/write.js:62:12)
    at Array.map (native)
    at /Users/patriklythell/Sites/sigh-test/node_modules/sigh/node_modules/sigh-core/src/stream.js:40:73

from file:

console.log('test);

with sigh file:

pipelines['build-source'] = [
    glob({ basePath: 'src' }, '**/*.js'),
    write('build/assets')
  ]
insidewhy commented 9 years ago

Thanks for that! This is not an error in a sigh plugin but comes from sigh's internals. In this case sigh has to produce an identity source map (as no transformations happen, the identity source map can allow the moved file to map onto itself). I've missed some error checking in that process though, I'll have the fix ready in probably just a few minutes.

If you had any code transforms the error wouldn't have triggered as the identity source map wouldn't be needed, so it's not an error most people would see.

PatrikLythell commented 9 years ago

Ah, I see.

I guess that's not much of a real world scenario but thanks for providing some insights into error handling sigh as I am also dabbling in some plugin-creation.

Really loving Sighs intent, keep up the good work!

insidewhy commented 9 years ago

I've added the fix for your issue in 175cb35 but I don't have time to add a test for it right now. You can get this version by installing version 0.12.12-beta.1. If you could let me know if that fixes your issue that would be really helpful!

sigh is great ;) Sometimes it's so fast you think it's broken, then you realise it wrote your errors to the terminal faster than you could switch tabs.

PatrikLythell commented 9 years ago

It worked, but found the same error for concat plugin.

Used the same solution as yours for write plugin.