These are chat archives for jshttp/jshttp

29th
Oct 2014
Alan Plum
@pluma
Oct 29 2014 00:10
:+1:
Douglas Wilson
@dougwilson
Oct 29 2014 00:11
cool. now I wonder if it's possible to delete a room, lol
Alan Plum
@pluma
Oct 29 2014 00:11
Not really, but you can leave it.
Douglas Wilson
@dougwilson
Oct 29 2014 00:11
gitterHQ/gitter#218 so, nope :)
OK, anyway, to http-errors stuff
Alan Plum
@pluma
Oct 29 2014 00:13
github.flavoured.markdown = works.too;
Yes.
Douglas Wilson
@dougwilson
Oct 29 2014 00:15
Darn, there are two PR merges in the history :(
Alan Plum
@pluma
Oct 29 2014 00:15
Hm?
Doesn't GitHub do that whenever you merge a PR anyway?
Douglas Wilson
@dougwilson
Oct 29 2014 00:16
Right, but we don't let GitHub merge PRs :)
Alan Plum
@pluma
Oct 29 2014 00:16
Ah
Well, @jongleberry does.
Douglas Wilson
@dougwilson
Oct 29 2014 00:17
Only a couple times on accident :)
Alan Plum
@pluma
Oct 29 2014 00:18
Well, now it's too late for that. No use crying over spilt milk.
Douglas Wilson
@dougwilson
Oct 29 2014 00:19
git history can be rewritten easily when i feel like doing it
Alan Plum
@pluma
Oct 29 2014 00:19
sure, if you don't mind changing all the tags.
Douglas Wilson
@dougwilson
Oct 29 2014 00:20
lol. i do it all the time. I have scripts for that stuff. Several of the jshttp modules have had their history rewritten.
https://github.com/jshttp/range-parser was the most recent, I believe
Alan Plum
@pluma
Oct 29 2014 00:20
Fair enough, I guess.
Anything in particular you wanted to talk about?
Douglas Wilson
@dougwilson
Oct 29 2014 00:21
Just mainly about your thoughts on http-errors. What do you think are it's short-comings? Rough edges? etc.
Alan Plum
@pluma
Oct 29 2014 00:22
Let me see if I can dig up the original chatlog.
Alan Plum
@pluma
Oct 29 2014 00:27

I think this is the interesting bit:

the main problems I currently see are: 1. constructor.name is meaningless, 2.err.name isn't set (giving lame stack traces) and 3. it uses implementation artifacts (captureStacktrace and __proto__) making it incompatible with some environments.

Douglas Wilson
@dougwilson
Oct 29 2014 00:28
for 3, we could add feature detection at runtime for compatibility. would that be good enough?
Alan Plum
@pluma
Oct 29 2014 00:29
I think captureStacktrace is called automatically the first time you access err.stack.
Douglas Wilson
@dougwilson
Oct 29 2014 00:30
no
it's purpose is because otherwise, the stack will point to the line with "new Error()" called
Alan Plum
@pluma
Oct 29 2014 00:30
Ah
Douglas Wilson
@dougwilson
Oct 29 2014 00:30
which is inside our module and has nothing to do with the error
Alan Plum
@pluma
Oct 29 2014 00:30
Okay, that makes sense. I work around that in httperr: https://github.com/pluma/httperr/blob/1.0.0/index.js
Douglas Wilson
@dougwilson
Oct 29 2014 00:30
captureStacktrace allows us to alter where the top of the stack is, typically the place that called us :)
Alan Plum
@pluma
Oct 29 2014 00:31
But I guess doing if (Error.captureStatcktrace) Error.captureStacktrace(...) would be fine.
Douglas Wilson
@dougwilson
Oct 29 2014 00:31
Yea. the work-around you have there involves assuming a specific stringification of the stack
Alan Plum
@pluma
Oct 29 2014 00:32
I know. I'm not happy with my hack.
I wasn't aware captureStacktrace does the same. The rest is just so I can have nested stacktraces for nested errors.
Douglas Wilson
@dougwilson
Oct 29 2014 00:33
(p.s. if you're interested in how v8 does stack trace string building: https://github.com/dougwilson/nodejs-depd/blob/master/lib/compat/callsite-tostring.js)
anyway, sorry I was just finishing reading through the IRC log
Alan Plum
@pluma
Oct 29 2014 00:35
Do we need to use arguments.callee? Isn't that just a reference to the named function?
Douglas Wilson
@dougwilson
Oct 29 2014 00:36
It is. That line could easily be Error.captureStackTrace(self, ServerError) (and ClientError for the other).
Alan Plum
@pluma
Oct 29 2014 00:37
Okay, that's not so bad then.
Only problem is we have to rely on __proto__ assignment to make isError and toString behave.
My PR fixes point 2, so only 1 is left.
Douglas Wilson
@dougwilson
Oct 29 2014 00:39
So, I don't do a lot of client-side stuff. What happens if that is done in a browser that doesn't support __proto__?
Alan Plum
@pluma
Oct 29 2014 00:40
Nothing. Worst case: either it has no effect or the object gets a property named __proto___.
Basically, if the environment doesn't support assigning __proto__, you just get an object that fails instanceof checks.
Douglas Wilson
@dougwilson
Oct 29 2014 00:41
Ah, yea, the instanceof wouldn't work, then. hm
Alan Plum
@pluma
Oct 29 2014 00:41
No real way to fix that. Probably should mention that limitation in the README, though. But util.isError seems more important.
Douglas Wilson
@dougwilson
Oct 29 2014 00:42
So the other thing, is if we reverse it so we make it a true new ServerError, then it wouldn't have a stack property
But we could add one as long as Error.captureStackTrace is supported
Alan Plum
@pluma
Oct 29 2014 00:43
No, that's not a problem. We can copy all of that over (including the non-standard properties set in other environments like IE)
Douglas Wilson
@dougwilson
Oct 29 2014 00:43
But then stack is just a string, instead of being a magical memory-saving getter
Alan Plum
@pluma
Oct 29 2014 00:43
oldIE doesn't support captureStacktrace, but it doesn't support stack either.
Yes.
But you said util.isError and Object.prototype.toString are more important, so the point is moot anyway.
Douglas Wilson
@dougwilson
Oct 29 2014 00:45
i.m.o. if we have proper name properties, then you don't need instanceof checks; I personally always prefer to check err.name instead of instanceof, because you don't always have a way to get a reference to the error constructor.
And you can emulate multiple catch block support with try { ... } catch (err) { switch (err.name) { ... } }
and even add a default: throw
Alan Plum
@pluma
Oct 29 2014 00:47
@rlidwka just argued against havig err.name be the constructor name.
Douglas Wilson
@dougwilson
Oct 29 2014 00:47
I know. But I don't necessarily agree :)
I think long errors names are A-OK, since it's what the JS language gives us to work with.
We have lots of time to think, because this may be a major version bump anyhow
I hate JS's error handling, since it seems so half-hearted and I don't think ES6 even touched on it (or even ES7)
We could kind of support inheritance if err.name === 'NotFoundError' && err instanceof ClientError or something, idk
Alan Plum
@pluma
Oct 29 2014 00:52
But that would only work in environments where __proto__ can be set, again ;)
Alex Kocharin
@rlidwka
Oct 29 2014 00:54
What environments don't support proto? Is that some kind of microsoft issue again? :)
Douglas Wilson
@dougwilson
Oct 29 2014 00:54
I know, lol. But you'd still have err.name, which is usually what you would switch on, right? Unless we think it's more common to switch on server vs client errors
Alan Plum
@pluma
Oct 29 2014 00:54
Well, in ES6 you should use Object.setPrototypeOf anyway but: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
Douglas Wilson
@dougwilson
Oct 29 2014 00:55
@rlidwka just browsers in general. Some of these modules are border line browser-compatible or something
Alex Kocharin
@rlidwka
Oct 29 2014 00:55
yeah I remember some people asking to change "string"[0] to "string".chatAt(0) 'cause first one isn't portable.
Douglas Wilson
@dougwilson
Oct 29 2014 00:56
The MDN article says all browsers support __proto__, though I'm sure there are certain versions where they actually appeared
i.e. IE10+ supports it
but IE9 and lower don't.
Douglas Wilson
@dougwilson
Oct 29 2014 00:57
It'd be nice if browserify let us not care. Does it?
Alan Plum
@pluma
Oct 29 2014 00:58
Browserify can't fix it. How could it?
There's no workaround for assigning to __proto__.
Douglas Wilson
@dougwilson
Oct 29 2014 00:58
IDK :)
Alan Plum
@pluma
Oct 29 2014 00:58
Despite appearances suggesting the contrary, browserify is not actually magic.
Alex Kocharin
@rlidwka
Oct 29 2014 00:59
There is also Object.create(Error.prototype). Does IE support that?
Alan Plum
@pluma
Oct 29 2014 00:59
You're kidding, right?
Douglas Wilson
@dougwilson
Oct 29 2014 00:59
I don't think that Object.create handles [[Class]], though
Alan Plum
@pluma
Oct 29 2014 01:00
Nope, just tried it. [object Object]
Alex Kocharin
@rlidwka
Oct 29 2014 01:00
oh right... it doesn't
Douglas Wilson
@dougwilson
Oct 29 2014 01:00
JS sucks, you guys :)
Alan Plum
@pluma
Oct 29 2014 01:00
Besides, it doesn't and the polyfill is using new.
JS sucks, let's switch to Go.
Okay, but seriously now...
Jarvis Badgley
@ChiperSoft
Oct 29 2014 01:01
hey, fancy that
hmm, no backlog on the gitter irc
Alan Plum
@pluma
Oct 29 2014 01:01
What do you mean?
Douglas Wilson
@dougwilson
Oct 29 2014 01:02
@ChiperSoft connected via an IRC proxy
Alan Plum
@pluma
Oct 29 2014 01:02
Ah
Yeah, it can't do that.
Jarvis Badgley
@ChiperSoft
Oct 29 2014 01:02
well, it could, it just doesn't
Alan Plum
@pluma
Oct 29 2014 01:02
Your IRC client would have to fetch the chatlog from the server and display that.
Jarvis Badgley
@ChiperSoft
Oct 29 2014 01:02
znc does backlog just fine
Alan Plum
@pluma
Oct 29 2014 01:02
Huh. Never saw that.
Jarvis Badgley
@ChiperSoft
Oct 29 2014 01:03
if I reconnect to my znc account, all the channels on freenode will output the last 100 messages
Alan Plum
@pluma
Oct 29 2014 01:35
I've updated the PR to use the NotFoundError naming (with deduplication of the suffix) and submitted a PR for getting rid of arguments.callee.
Erp, it broke.
Douglas Wilson
@dougwilson
Oct 29 2014 01:38
Just change the name instead of messing with the message in the same PR :)
Alan Plum
@pluma
Oct 29 2014 01:39
So you want NotFoundError: Not Found?
Douglas Wilson
@dougwilson
Oct 29 2014 01:39
sure, for now. we can always talk about changing the default message in another
no reason to bundle a bunch of changes together to get some through. we're not Congress ;)
Alan Plum
@pluma
Oct 29 2014 01:45
Okay, both PRs are green.
Douglas Wilson
@dougwilson
Oct 29 2014 01:45
sweet :)
Alan Plum
@pluma
Oct 29 2014 01:48
I think once the PRs are gone, I'll make a PR to add the missing semicolons. The repo is currently a bit inconsistent in its semicolon use but seems to be in favour of them rather than against them.
Actually I think I'll do that now.
Douglas Wilson
@dougwilson
Oct 29 2014 01:54
The style does seem to indicate to use semicolons, especially since the docs do.
Alan Plum
@pluma
Oct 29 2014 02:00
@dead-horse is secretly Yoda.
Alan Plum
@pluma
Oct 29 2014 02:06
@rlidwka Okay, should I remove all semicolons or what? Currently it's wildly inconsistent.
Alex Kocharin
@rlidwka
Oct 29 2014 02:07
Probably not, as far as I know we don't do any style-only changes.
Alan Plum
@pluma
Oct 29 2014 02:07
I'm not changing the style, I'm making it consistent. If the jshttp projects aren't using semicolon-free syntax, those semicolons are missing.
And it doesn't seem like a good idea to just fix that in one of my other PRs, hence the separate PR.
Alex Kocharin
@rlidwka
Oct 29 2014 02:10
jshttp projects do use semicolon-free syntax, see https://github.com/jshttp/style-guide/blob/master/javascript.md . But there is a lot of old code where they are still used.
Alan Plum
@pluma
Oct 29 2014 02:11
So the semicolons were added in error, then, and I should remove them?
I don't have a dog in the race. I just can't stand the inconsistent use of semicolons as it is now.
Alex Kocharin
@rlidwka
Oct 29 2014 02:12
If you're writing new code, it's better to remove them. But changing existing code ain't worth it, 'cause screws up git blame, etc.
jongleberry
@jonathanong
Oct 29 2014 02:13
yeah we care about git blame more than the actual styles
Alan Plum
@pluma
Oct 29 2014 02:13
Okay, fair enough. I'll try to get over my OCD then.
Alex Kocharin
@rlidwka
Oct 29 2014 02:19
What's with arguments.callee there? Does it cause any performance issues?
Douglas Wilson
@dougwilson
Oct 29 2014 02:20
it's usage prevents the function from being optimized at all
Alan Plum
@pluma
Oct 29 2014 02:21
Well, for one thing, arguments.callee is deprecated, for another it means the function can't be optimized at all.
Douglas Wilson
@dougwilson
Oct 29 2014 02:21
(i.e. it's a bail out condition for v8 optimizer)
Alan Plum
@pluma
Oct 29 2014 02:21
that ^
jongleberry
@jonathanong
Oct 29 2014 02:21
@dougwilson that alone convinced me to remove it
haha
lol
wtf
i added it
Alan Plum
@pluma
Oct 29 2014 02:23
uwotm8?
jongleberry
@jonathanong
Oct 29 2014 02:23
i'm pretty sure i based it off someone else's work
just copied pasta
Douglas Wilson
@dougwilson
Oct 29 2014 02:24
right. it's just a copy paste most likely, because I always see Error.captureStackTrace used with arguments.callee in examples so people don't have to explain what the second argument can be :)
jongleberry
@jonathanong
Oct 29 2014 02:24
imo as long as we get a stack trace working then i don't really mind removing arguments.callee
Alan Plum
@pluma
Oct 29 2014 02:25
We don't actually use captureStackTrace in the ad-hoc errors. Is that intentional?
Douglas Wilson
@dougwilson
Oct 29 2014 02:26
should be added to them :)
I tested the other errors with the PR merged:
$ node -pe 'new require("./index").NotFound().stack'
Error: Not Found
    at [eval]:1:24
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:536:25)
    at startup (node.js:80:7)
    at node.js:906:3
Alex Kocharin
@rlidwka
Oct 29 2014 02:26
Depends on what you call ad-hoc errors. But captureStackTrace is there to remove ServerError itself from the stack trace.
Alan Plum
@pluma
Oct 29 2014 02:27
@rlidwka I mean the exported function.
@dougwilson eval?
Douglas Wilson
@dougwilson
Oct 29 2014 02:27
here is what the "ad-hoc" errors produce:
$ node -pe 'new require("./index")(404).stack'
Error: Not Found
    at module.exports (nodejs-http-errors\index.js:38:16)
    at [eval]:1:23
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:456:26)
    at evalScript (node.js:536:25)
    at startup (node.js:80:7)
    at node.js:906:3
Alex Kocharin
@rlidwka
Oct 29 2014 02:28
oh right, that's a bug
Douglas Wilson
@dougwilson
Oct 29 2014 02:28
yep
Alan Plum
@pluma
Oct 29 2014 02:28
I'll make a PR.
Douglas Wilson
@dougwilson
Oct 29 2014 02:29
I think we can call that the "function interface" and the others the "classical interface"
*"functional interface"
I need to add a history file to this repo, too
@pluma re: eval, that's just the source when you use the -e switch
Alan Plum
@pluma
Oct 29 2014 02:35
ah
Alan Plum
@pluma
Oct 29 2014 02:45
Okay, everything is green again.
Alan Plum
@pluma
Oct 29 2014 03:03
I'm out for the night. See ya.
jongleberry
@jonathanong
Oct 29 2014 04:01
thanks for your help!
i love contributors :D
Alan Plum
@pluma
Oct 29 2014 04:01
np
Jarvis Badgley
@ChiperSoft
Oct 29 2014 15:23
@dougwilson: I found a new edge case for deoptimization, at least in node 0.10
if any of the arguments are objects, instant KO
hmm, ok, maybe not
Jarvis Badgley
@ChiperSoft
Oct 29 2014 15:34
https://gist.github.com/ChiperSoft/f51b8e8fd6005095dbf1 it only deopts if the first time I call the function has objects attached
Jarvis Badgley
@ChiperSoft
Oct 29 2014 15:41
ok, that's enough with this rabbit hole.
jongleberry
@jonathanong
Oct 29 2014 15:45
lol
Alan Plum
@pluma
Oct 29 2014 17:04
node-sass has a bad linebreak in its CLI script and now published two versions with an unusable bin file :frowning:
Woe is me...
Also, immutable@3.0.0's cursor module contains untranslated arrow functions, preventing me from porting my code that uses it.