-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Simpler error handling #6
Conversation
I'm not sure about this one since I remember that the errors reported by browserify were not too helpful. I think we ran into this with coffeeify where the detailed parse errors got lost and much less useful errors got reported instead. This may have changed by now, but I'm not sure. |
The synchronous, buffered interface should just throw, because that's the simplest thing to do. The streaming interface should just emit 'error' events. Browserify can then handle those as appropriate and decide what to output.
I had to fix the tests to make them run on windows and then I also found that the mappings outputted didn't match the ones in the test, so I just skipped that part of the test. If you want to fix the tests properly I'm happy to try and help, but the error handling test does work. It's possible that the bug you are referring to is browserify/browserify#375 which has now been fixed. The issue there was that browserify was emitting syntax errors on the wrong stream so they couldn't be properly handled. |
Thanks for making this work on windows. Interesting about the source map tests, @domenic just changed the tests after switching to the official traceur since the source maps output didn't match. Maybe he can give some insight, possibly there was a patch done to traceur? Could you please add the actual sourcemap output that you get instead of short circuiting them? I'd like to see if it I'll get the same results on a Mac and on travis. |
Dude @ForbesLindesay turn off your autocrlf :P. Checkout as-is, commit LF only. The tests passed fine on my Windows without any normalization steps :). As for the source map output, yeah I assume that Traceur updated the source maps it outputs e.g. by transpiling better or something. I do think testing the exact source map is pretty fragile in the face of upstream changes (in the many dependencies that are involved in producing the maps), and probably it isn't a great idea to test them. |
I can see how testing mappings for exact content is fragile, so @ForbesLindesay maybe change those tests to only test that source mappings are there (i.e. have a length). You probably could ammend this commit to do that and remove the windows line ending related changes from it as well please. |
Thanks, this looks good now, will merge this and the related PR in a bit. |
Thanks, both went out with v0.4.0. |
The synchronous, buffered interface should just throw, because that's the simplest thing to do.
The streaming interface should just emit 'error' events. Browserify can then handle those as appropriate and decide what to output.