These are chat archives for redfin/react-server

18th
May 2016
Bo Borgerson
@gigabo
May 18 2016 20:15
:wave:
Erik Sjaastad
@eriksjaastad
May 18 2016 20:15
:wave:
Douglas Wade
@doug-wade
May 18 2016 20:15
Last night I started hacking on redfin/react-server#12 and redfin/react-server#37 While I was writing unit tests for the gulp plugin, I came to the opinion that we could remove it altogether by assigning a color randomly and getting the caller from the stack trace, which I think would significantly simplify client code and client builds. I think the migration path is fairly simple, since existing clients can continue using the tagger, and we'll just discard the arguments
Does that approach seem reasonable?
Bo Borgerson
@gigabo
May 18 2016 20:17
The module tagger:
  1. Provides consistent colors between server and browser.
  2. Has zero-overhead (at runtime) prefixing.
Stack traces are expensive.
Douglas Wade
@doug-wade
May 18 2016 20:18
We only have to produce the stack trace once per module, when the client calls getLogger
Bo Borgerson
@gigabo
May 18 2016 20:19
Are you able to reconstruct the module path from the stack trace browser-side?
Douglas Wade
@doug-wade
May 18 2016 20:36
Yes -- I'm helping a coworker at the office; I'll be back in a minute
Douglas Wade
@doug-wade
May 18 2016 21:00
Bo Borgerson
@gigabo
May 18 2016 21:01
Nice!
Douglas Wade
@doug-wade
May 18 2016 21:01
On the server, I use process.cwd() to trim it down
I can't do the same in the browser, so its less terse than before
It does also tie us to webpack more strongly, which I think we don't like?
Bo Borgerson
@gigabo
May 18 2016 21:01
/shrug
process.cwd seems like not what we want.
Shouldn't it be relative to the filename of the module responsible for trimming?
Douglas Wade
@doug-wade
May 18 2016 21:30
I think you're right that process.cwd isn't what we want
I'm not sure if we want our identifiers to be relative to the logger, though, either.
Bo Borgerson
@gigabo
May 18 2016 21:31
Ideally the identifiers wouldn't change with the new implementation.
Nor would the colors. :smiling_imp:
Douglas Wade
@doug-wade
May 18 2016 21:32
If we don't change identifiers, the colors won't change
Bo Borgerson
@gigabo
May 18 2016 21:32
:thumbsup:
Douglas Wade
@doug-wade
May 18 2016 21:32
We calculate the color from the hash of the identifier
Bo Borgerson
@gigabo
May 18 2016 21:32
You mentioned "random" earlier.
Douglas Wade
@doug-wade
May 18 2016 21:35

I cheated a little and changed my approach based on

Provides consistent colors between server and browser.

Bo Borgerson
@gigabo
May 18 2016 21:35
:relieved:
Douglas Wade
@doug-wade
May 18 2016 21:35
:wink:
But I'm not quite there yet
Because I'm using the filepath as the identifier in the browser, it has a different hash than the server
So you don't have consistent colors
I'll head back to the woodshed to figure that out
Bo Borgerson
@gigabo
May 18 2016 21:38
Cool. Definitely would be nice to get rid of the module tagger.
Douglas Wade
@doug-wade
May 18 2016 21:38

I would like to poke at your 2 for a quick sec, though

Has zero-overhead (at runtime) prefixing.

Do you mean that my plan to run console.trace once for every getLogger call is verboten?
I'm not sure what runtime means in this case
Or is that parse time?
Bo Borgerson
@gigabo
May 18 2016 21:39
Startup time seems fine, I guess.
How many loggers do we have?
Douglas Wade
@doug-wade
May 18 2016 21:39
:relieved:
Bo Borgerson
@gigabo
May 18 2016 21:40
Fewer than 100, generally, I'd guess.
Oh, the other thing to look at with stack traces is making sure to get them in a cross-browser-compatible way. :grimacing:
Douglas Wade
@doug-wade
May 18 2016 21:41
Oh man I only looked at chrome
I have no idea if ff/ie/edge et al implement console.trace
Bo Borgerson
@gigabo
May 18 2016 21:42
Suddenly source transforms aren't looking so bad, huh? :japanese_ogre:
Douglas Wade
@doug-wade
May 18 2016 21:42
:squint:
:notsureiftrolling:
The thing I don't like is that we tie all of our customers to using gulp or the cli, which uses gulp
The other approach I considered was making the gulp plugin call a cli that did the source transform
Bo Borgerson
@gigabo
May 18 2016 21:45
This will instead tie all of our customers to using webpack.
Choose your poison?
Douglas Wade
@doug-wade
May 18 2016 21:55
All of our customers are already tied to using webpack?
Bo Borgerson
@gigabo
May 18 2016 21:56
And gulp?
Douglas Wade
@doug-wade
May 18 2016 21:56
I would be more strongly tying them to webpack, though, yes
Bo Borgerson
@gigabo
May 18 2016 21:57
Another consideration: Exposing __filename in our bundles... security issue?
Douglas Wade
@doug-wade
May 18 2016 21:57
security issue?
Bo Borgerson
@gigabo
May 18 2016 21:58
Absolute paths could leak info on build environment.
gigabo @gigabo might be a little paranoid.
Bo Borgerson
@gigabo
May 18 2016 22:29
Do ticket mentions do anything cool here? Like... #173
Meh.
Douglas Wade
@doug-wade
May 18 2016 22:39
issues? redfin/react-server#170
same as prs.
Douglas Wade
@doug-wade
May 18 2016 23:22
Not quite the right place, but does anyone have any thoughts about redfin/request-local-storage#2 ?
Bo Borgerson
@gigabo
May 18 2016 23:23
Yes.
Haven't had a chance to gather them.
I don't agree with the "particular problem" designation.
That is a problem that RLS addresses.
It's not its reason for existence.
But that's as far as I've gotten.
... the first sentence? :grin:
Douglas Wade
@doug-wade
May 18 2016 23:25
:ok_hand: no hurries, no worries