rollup / rollup-watch

Fast incremental rebuilds with Rollup CLI
MIT License
91 stars 23 forks source link

Error catching #19

Closed zwhitchcox closed 8 years ago

zwhitchcox commented 8 years ago

Just curious as to why we are emitting errors as an event as opposed to returning a promise or logging them? Just spend about 2 hours trying to find an error without realizing it was going to emit an error event...

zwhitchcox commented 8 years ago

I would propose returning the rollup promise...I would also propose allowing people to pass in the rollup function, because most people import it like this:

import {rollup} from rollup

Maybe detect if rollup is a function, and if so use it as a function...I don't see anywhere in the code where we need the actual reference to the object, so, if so, that would be nice.

zwhitchcox commented 8 years ago

Also,

import rollup from 'rollup'

returns an error, so I don't even know how you're getting the rollup object

TrySound commented 8 years ago

@zwhitchcox To import rollup use

import * as rollup from 'rollup';

We won't improve api with such conditions. API should be clean and simple. But I think would be better to pass rollup function instead.

/cc @Rich-Harris

About promises. You can resolve it just once. But watcher is iterated again and again. So your propose is not the case. Logging works when you use CLI. In this case it's the same as logging promise rejection.

zwhitchcox commented 8 years ago

So, we have to use the CLI or listen for the error even if we want to get the errors?

You're saying the promise is only going to catch the error once, but writing

.then(()=> {}, error => console.log(error))

is the same as writing

.then(()=>{})
.catch(err => console.log(err))

Correct?

Or am I missing something here?

TrySound commented 8 years ago

Yep, it's the same. And both will be called once. With event emitter you may call listeners any times.

TrySound commented 8 years ago

Different API for different purposes.

zwhitchcox commented 8 years ago

gotcha...modified my pr...how's that?

will detect if run by command line, and if not, will output error....

rollup already shows the error twice for me, don't want a third time XD

TrySound commented 8 years ago

@zwhitchcox It's already done in CLI https://github.com/rollup/rollup/blob/master/bin/src/runRollup.js#L162-L194

zwhitchcox commented 8 years ago

Yes, but it doesn't log the error withthe programmatic api, e.g.:

  1 // I roll up, I roll up, I roll up, Shawty I roll up                                                                                    
  2 import * as rollup from 'rollup'                                                                                                        
  3 import watch from 'rollup-watch'                                                                                                        
  4 import config from '../rollup.config.js'                                                                                                
  5 watch(rollup, config)                                                                                                                   
  6   .on('event', e => e.code === 'ERROR' && console.log(e))    

unless you listen for the event...or is this the intended behavior...also, +1 for this not being the intended behavior haha

talmobi commented 8 years ago

In my small watcher wrapper I directly use rollups js API (without rollup-watch) and catch the errors from the rollup.rollup promise directly. Seems to work pretty OK since a rebuild is done on each change (using cache if available of course).

(and onwarns that pass if ( /Treating .+ as external dependency/.test( message ) ) return

2cents

zwhitchcox commented 8 years ago

Thank you!

Yeah, I actually did get rollup-watch to work, but now there's other problems with rollup that I can't figure out :|...So, I think I'm just gonna make my life a whole lot easier and use jspm, because I'm not developing a library XD