code cleanup and other changes #20

Closed
wants to merge 46 commits into
from

2 participants

@defunctzombie

I am using this library in a number of my projects and wanted to offer some changes which I think give the library a bit more focus and also fix some things. I have outlined some of the changes below and some more plans

  • Remove express/connect stuff. I would make a separate module for this. Keep this module simple and focused on just talking to getsentry
  • read modules from main module path (this is actually a fix. You can't rely on the cwd since apps don't need to be launched from the folder their project is in. Better to use require.main
  • pin node-uuid version (be nice to users of your package, don't make their deployments vary depending on what the latest version of a package might be. Any changes could potentially break things and since you test against a specific version, depend on that version).

Plans

  • remove NODE_ENV variable use. It is not for you to decide what variables constitute "production" this is a faux convention that was propagated in the early days of node that really has no bearing on usecase.
  • cleanup some of the code to be more javascript style and less python like (object access, etc)
  • remove global catch all. Again, users can write other modules that do this (book, winston, loggly) and handle it how they want. Better to keep this module simple.
  • remove auto recognition of the other env variables. Goes along with explicit better than implicit. Maybe suggest the use of these variables or example patterns but otherwise don't autodetect.

Basically, the my idea is that this module should really do nothing more than help you create messages in the proper format and send them along. More sophisticated logging modules can be built on top of this one which do some of the more "opinionated" things.

I am happy to hop on IRC to chat about why I feel these changes are good :)

defunctzombie added some commits Sep 14, 2012
@defunctzombie defunctzombie pin node-uuid version
deploying unpinned things is bad. If node-uuid get an incompatible API
change then it will break for new deployments during runtime!
cafd86d
@defunctzombie defunctzombie remove Makefile
move test execution to npm script 'test' This is more cross platform
compatible and achieves the same result without the extra makefile
f7a8791
@defunctzombie defunctzombie code tidying
remove _ alias and just type of prototype. Less reading and variables.
6dfc872
@defunctzombie defunctzombie remove connect/express middleware
Keep the module code simple and focused on sending errors to getsentry.
If more specialized functionality is desired it can be provided by other
modules building on this one.
b31abe0
@defunctzombie defunctzombie read modules from the path of the main module
Do not assume that the module directory is the 'cwd'. This is not the
case when starting node apps from outside of the project directory.
Using require.main.paths allows raven to access the paths node will use
to look for modules to load.
372d48c
@defunctzombie defunctzombie bump version to 1.0.0-pre
development on what will be version 1.0.0 since I plan to do some more
serious API changes.

connect/express API has already been removed
c6990ab
@defunctzombie

Oh yea, I should also add that I bumped the version to 1-pre because the changes break some previous API. I think a wiki which explains how to migrate if these changes are accepted (and future changes) would be fine.

@mattrobenolt
Sentry member

Sure, let's chat on IRC about these this evening. I'm on board with a lot of them, and you've raised some thought about some other things as well.

@defunctzombie

@mattrobenolt Sounds good. I will hop on IRC later this evening and look for you there.

defunctzombie added some commits Oct 10, 2012
@defunctzombie defunctzombie export parsers 3ee4026
@defunctzombie defunctzombie setting a valid DSN in enough to enable logging
Don't do magic with environment variables. Leave this up to the user. If
they don't want logging then they can avoid creating a client or
something more appropriate to their environment.
cb6f71b
@defunctzombie defunctzombie remove unused require a1c119c
@defunctzombie defunctzombie remove patchGlobal
Leave this up to the application or other logging framework. Some users
will want to handle this with 'once' versus 'on'. Keep module focused on
interfacing with sentry instead of being a general purpose catch all
logger.
c5f8bfa
@defunctzombie defunctzombie backwards compat for dsn false value
If the DSN is false then assume the user meant to disable logging and
don't print the warning.
ad8db8b
@defunctzombie defunctzombie move dsn parsing to separate file
Down with utils!!
7f02be7
@defunctzombie defunctzombie error parsing using stackback 9edf32a
@defunctzombie defunctzombie code style cleanup 47e780d
@defunctzombie defunctzombie separate transports and dsn parsing
DSN parsing independent of transport verification. Client checks for
valid transport and setting up the desired transport given dsn parsing.
4544e63
@defunctzombie defunctzombie down with utils!
Moved lib/utils functions to better named files.
180138e
@defunctzombie defunctzombie make request parser generic
Not tied to being used with connect/express.
 - parse cookies if not already parsed
 - grab body from req in available
13fd789
@defunctzombie defunctzombie separate parsers into interfaces b15bc6e
@defunctzombie defunctzombie remove NODE_ENV documentation
Raven does not depend on the NODE_ENV environment variable. It is left
up to the user to decide when to activate raven.
ad953f9
@defunctzombie defunctzombie remove SENTRY_SITE variable
deprecated in favor of tags
5323c58
@defunctzombie defunctzombie export interface names for reference 24deb2e
@defunctzombie defunctzombie change interface name export to 'key'
'name' cannot be assigned on the function because it is already a
property.
b88c106
@defunctzombie defunctzombie bump version 2da3b53
@mattrobenolt mattrobenolt added a commit that referenced this pull request Nov 10, 2012
@mattrobenolt mattrobenolt Remove Makefile, refs #20 ec69890
defunctzombie added some commits Dec 30, 2012
@defunctzombie defunctzombie deprecate captureQuery and remove from docs
This is outside the scope of the project and is not encouraged for use.
3ba4e6c
@defunctzombie defunctzombie use lsmod to capture runtime module information
- add test for capturing module info
0367467
@defunctzombie defunctzombie better capture of http response from sentry server
Capture the result body for diagnostic information if user wants it. It
is available in the .body field of the error.
6a9c3e6
@defunctzombie defunctzombie attack packet to client errors befor emitting
Allows for more complete diagnostics.
eba85b3
@defunctzombie defunctzombie update client module test 8674c53
@defunctzombie defunctzombie update description 4585b4c
@defunctzombie defunctzombie 1.0.0-dz1 29eea21
@defunctzombie defunctzombie remove captureQuery from readme 2b68085
@defunctzombie defunctzombie fix stacktrace capture
use stack-trace to just parse stacktrace string
ead55cc
@defunctzombie defunctzombie fix test for stacktrace interface 1de3a8d
@defunctzombie defunctzombie interfaces: unset lineno from stacktrace frame if not available
Sentry does not like null values for lineno
f69aef8
@defunctzombie defunctzombie interfaces: ignore empty body on http request d4dd254
@defunctzombie defunctzombie 1.0.0-dz2 91fdad2
@defunctzombie defunctzombie update to lsmod 0.0.2
fixes module lookup
bf78e4b
@defunctzombie defunctzombie 1.0.0-dz3 e0da769
@defunctzombie defunctzombie remove typename from stacktraces
Having a typename would break sentry even tho the field is not used.
d485f27
@defunctzombie defunctzombie 1.0.0-dz4 b77d127
@defunctzombie defunctzombie update travis.yml with node 0.10 930574a
@defunctzombie defunctzombie fix DSN parsing to include leading / in path 27973f2
@defunctzombie defunctzombie ignore circular references in packet e08e5f4
@defunctzombie defunctzombie update and pin dev deps 55ba95d
@defunctzombie defunctzombie update dependencies
- switch to uuid module
74d203f
@defunctzombie defunctzombie 1.0.0-dz5 e377346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment