1 | # Guidelines for contributing to trace-nodejs
|
2 |
|
3 | RisingStack welcomes contributions for the trace-nodejs package by anyone who'd
|
4 | like to make it better!
|
5 |
|
6 | To make everyone's life easier, please follow this guidelines on how you can do
|
7 | that.
|
8 |
|
9 | ## Legal
|
10 |
|
11 | By sending pull request you agree to grant RisingStack, Inc. perpetual,
|
12 | irrevocable, non-exclusive, worldwide, no-charge, royalty-free, unrestricted
|
13 | license in the code, algorithms, idea, object code, patch, tool, sample,
|
14 | graphic, specification, manual or documentation (“Your Contribution”) to
|
15 | reproduce, publicly display, publicly perform, sublicense, and distribute Your
|
16 | Contribution and such derivative works. You provide your contributions on an "as
|
17 | is" basis, without warranties of any kind, including warranties of correctness
|
18 | or fitness for a particular purpose.
|
19 |
|
20 | ## Code compatibility
|
21 |
|
22 | The library code is required to run on target platforms Node.js >=v0.10 (Node >=
|
23 | v0.12 for 3.x.x) without transpilation. This means the code should fully adhere to the
|
24 | ES5 standard: **Use only ES5 compatible language features**.
|
25 |
|
26 | Most notable gotchas without completeness:
|
27 |
|
28 | - No classes with `class`
|
29 | - No arrow functions
|
30 | - No object method shorthand
|
31 | - No property assignment shorthand
|
32 | - No rest parameters, no destructuring
|
33 | - A lot of `Array` and `Object` methods you have in ES6 are missing from earlier versions.
|
34 |
|
35 | > When you are in doubt about the compatibility of a language specific feature check on [Mozilla Developer Network](mdn)!
|
36 |
|
37 | Moreover, the code must be compatible with target Node.js
|
38 | APIs. As these remain mostly backward compatible, (functionality is added,
|
39 | seldom removed) this boils down to finding the common subset of features and in
|
40 | turn looking at the earliest supported version's feature set.
|
41 |
|
42 | A few points to mention:
|
43 | - Sync APIs were introduced in v0.12, v0.10 misses these.
|
44 | - Streams API were largely extended in later versions.
|
45 |
|
46 | ## Source code style
|
47 |
|
48 | We use [ESLint](eslint) for code linting.
|
49 |
|
50 | ### ESLint config
|
51 |
|
52 | We use an ESLint configuration called [Standard Style](stdjs).
|
53 |
|
54 | An incomplete list of rules:
|
55 | - 2 spaces – for indentation
|
56 | - Single quotes for strings – except to avoid escaping
|
57 | - No unused variables – this one catches tons of bugs!
|
58 | - No semicolons
|
59 | - Never start a line with (, [, or \` This is the only gotcha with omitting
|
60 | semicolons – automatically checked for you!
|
61 | - Space after keywords
|
62 | - Space after function name
|
63 | - Always use `===` instead of `==` – but `obj == null` is allowed to check `null || undefined`.
|
64 | - Always handle the node.js `err` function parameter
|
65 |
|
66 | ### Functional vs imperative
|
67 |
|
68 | Code clarity is an important to us. Immutable data provides easier effect tracking
|
69 | therefore lesser cognitive overhead when understanding code of others. The
|
70 | fundamental program composition model of functional programming is based on
|
71 | function composition rather than state. However, keep in mind that Javascript is
|
72 | not a functional language. Using idiomatic language constructs even in presence
|
73 | of side effects enhances collaboration, and helps writing performant code. This
|
74 | requires you to keep a balance of imperative and functional code. The following
|
75 | rule of thumbs will help you:
|
76 |
|
77 | - encapsulate and segregate state. Don't keep
|
78 | stateful objects all over the place, rather in reservoirs. Use private
|
79 | properties (starting with an \_underscore) to encapsulate state that should not
|
80 | be accessed from outside.
|
81 |
|
82 | - use resourceful state in a controlled manner, for
|
83 | example provide state management functions for native timers.
|
84 |
|
85 | - Keep in mind that function calls in instrumentation wrapper code will be
|
86 | present in the stack trace if an error happens in the target library. So
|
87 | - keep function call chains as shallow as possible.
|
88 |
|
89 | ### Error tolerant instrumentation code
|
90 |
|
91 | It is very important that the
|
92 | instrumentation hooks are tolerant to misuse. We should expect errors to be
|
93 | present in user code interfacing with the instrumented libraries. In this case
|
94 | it should be left to the instrumented library to handle the error, so that our
|
95 | intermediate layer remains as transparent as possible.
|
96 |
|
97 | ## Code organization
|
98 | ```
|
99 | .
|
100 | ├── CHANGELOG.md
|
101 | ├── CONTRIBUTING.md
|
102 | ├── LICENSE
|
103 | ├── README.md
|
104 | ├── circle.yml
|
105 | ├── coverage
|
106 | ├── example There is an example app here.
|
107 | ├── lib Source code goes here, along with unit tests.
|
108 | ├── package.json
|
109 | ├── scripts Build, test, deployment scripts.
|
110 | └── test All tests except unit tests.
|
111 | ```
|
112 |
|
113 | Keep the unit tests next to their corresponding source files. This makes it
|
114 | easier to navigate between them.
|
115 |
|
116 | ## Guidelines about package dependencies
|
117 |
|
118 | Keep in mind at all times that the code will run on clients' machines. Every
|
119 | dependency consumes some disk space, adds memory overhead (when loaded), network
|
120 | overhead on install. Native modules make build times longer with a
|
121 | non-negligible amount (if they are allowed to be built at all). Additionally
|
122 | every new dependency or update adds the possibility to introduce a bug, a
|
123 | regression or a security hole. To minimize exposure to such adverse effects,
|
124 | following rules must be adhered to.
|
125 |
|
126 | ### Evaluation
|
127 |
|
128 | Before adding a dependency first ask yourself:
|
129 |
|
130 | #### How does this package help?
|
131 |
|
132 | Summarize why this particular package helps solving your problem.
|
133 |
|
134 | #### Is it really needed, can I achieve my goal without it? If I can what are the
|
135 | trade-offs (very complex, missing knowledge in domain)?
|
136 |
|
137 | Some packages do not simplify your problem too much. Those that add syntactic
|
138 | sugar for control structures, validation shortcuts, or those that try to solve a
|
139 | very complex general problem that you are facing may not be the best candidate
|
140 | for inclusion. However if it solves a specific hard mathematical or domain
|
141 | specific problem, eg. cryptographic or compression algorithms, protocol
|
142 | implementations, use it. Another particular kind is polyfills and lodash-like
|
143 | helpers, these are generally acceptable, just use them if you really need to.
|
144 |
|
145 | #### Can I find a more mature one? Better maintained, used by more projects, etc.
|
146 |
|
147 | This should go without further explanation.
|
148 |
|
149 | #### Is there a package covering my needs with smaller footprint? Less transitive dependencies, optional native deps, etc.
|
150 |
|
151 | Prefer modular packages, avoid kitchen sinks. For example, use lodash.find and
|
152 | lodash.defaults instead of depending on the whole lodash module, if you need
|
153 | only those two methods.
|
154 |
|
155 | Prefer flat packages, that is, avoid ones with big dependency subtrees. Remember the leftpad scandal.
|
156 |
|
157 | #### Is it a native module or does it include a native dependency? Is it optional?
|
158 |
|
159 | To build native dependencies the native compiler build chain is required as well
|
160 | as some additional dependencies. Often on production systems these are not
|
161 | present because of security reasons. Prebuilding can be done but complicates
|
162 | distribution, therefore currently we do not do it. This means currently every
|
163 | native dependency must be optional, so the software needs to function (possibly
|
164 | with limited feature set and/or performance) without them.
|
165 |
|
166 | ### Adding the dependency
|
167 |
|
168 | **Lock the dependency!** Production dependencies must be locked in package.json. This means that semver
|
169 | ranges are not allowed. The reason behind this is our experience with
|
170 | change management in npm packages. There is a possibility that they introduce
|
171 | breaking changes even on patch level, and it is always harder to provide support
|
172 | for a range of possible versions.
|
173 |
|
174 | [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript
|
175 | [eslint]: http://eslint.org/
|
176 | [stdjs]: http://standardjs.com/
|