Skip to content
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

Closed
wants to merge 3 commits into from
Closed

Simpler error handling #6

wants to merge 3 commits into from

Conversation

ForbesLindesay
Copy link
Contributor

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.

@thlorenz
Copy link
Owner

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.
I would be happier to merge this if there was a test that shows that detailed parse errors get logged to the console when this is used as a transform.

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.
@ForbesLindesay
Copy link
Contributor Author

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.

@thlorenz
Copy link
Owner

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.

@domenic
Copy link
Collaborator

domenic commented Aug 25, 2013

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.

@thlorenz
Copy link
Owner

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.

@thlorenz
Copy link
Owner

Thanks, this looks good now, will merge this and the related PR in a bit.

@thlorenz
Copy link
Owner

Thanks, both went out with v0.4.0.

@thlorenz thlorenz closed this Aug 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants