Suggest using eslint as a linter and plug this into the Circle CI build process.
https://github.com/Financial-Times/eslint-config-origami-component.
Suggest using airbnb's styleguide as it's very strict and hand-holdey, and provides good references as to why.
e.g the Next config:
root = true
[**{.js,.ts,.html,.hbs,.mustache,.xml,.xsl,.scss,.css,.sh,.vcl,.mk,Makefile,.json,.yml,.yaml,.md}]
end_of_line = lf
insert_final_newline = true
charset = utf-8
trim_trailing_whitespace = trueThis is a simple tool which enforces spacings and line endings. This should not conflict with either eslint (above) or prettier) below. This is particularly recommended due to developers in the team using a combination of OSX and Windows so likely having line-endings issues by default.
Using prettier should remove any stylistic arguments added by the previous step, and removes and cognitive overhead of correcting formatting, instead using editor plugins to do it on save.
The downsides here are the tooling overhead, as this may require using a tool such as lint-staged to work effectively.
Suggest trialing this depending on overhead of setup.
Add husky as an easy way to mange git hooks (added on npm install). Use this to try to prevent bad code getting to master.
This is an easy indicator of big regressions which should integrate with Circle CI. Just add nyc around any mocha calls and output to:
"./coverage/lcov-report/"with the lcov and/or html reporters.
Use npm scripts as a source of truth for running things. The alternative is another mature tool such as a Makefile, probably depends on comfort level in the team. Try to move away from inline scripts in CI configuration.
Move to NPM5 (specify this in engines), primarily for lockfiles but also for speed. The warts on the > 5 releases should have been removed by now.
I'd suggest yarn but it's less familiar and may not be compatible with all tooling.
Consider changing devDependencies to ^ ranges and dependencies to ~ ranges if they are using semver (basically anything but request/request from my personal experience).
Consider moving to v2 syntax if possible. Notify on master builds but not branch builds.
Require at least 1 approval on all changes into master, using github pull requests. Enable branch protection on master so it cannot be pushed to without approvals except with a manual override. This will also mandate a CI run on the feature branch so reduce breakage.
Pair more, especially on features or areas people aren't familiar with. Snags/bugs may not require this, be pragmatic.
Return any promises, do not leave them as side effects except when explicitly desired.
Name promise handlers, instead of
return doSomeAsyncWork()
.then((result) => {
... some stuff with result
});consider writing
function handleResult(result) {
... some stuff with result
}
return doSomeAsyncWork()
.then(handleResult);this format encourages separating functions into smaller chunks which can be named appropriately, and allows the promise chain to be read in a straightforwards, linear fashion by requiring less understanding of implementation details.
Never use the Promise constructor new Promise((resolve, reject) => except where entirely necessary, prefer using either Bluebird.promisifyAll or require('util').promisify (node > 7). This keeps everything in the same format, removes a level of nesting, and often removes a number of boilerplate function returns in the form of resolve() and reject().
Consider transpiling all code with babel and using async/await rather than functions using the Promise constructor.
Use other tools already made available at a global and org level. An immediately obvious candidate is next-logger.
Place source in a src and test in a test directory mirroring one another where possible.
Alternatively, use the jest style of a __test__ folder in each directory, e.g. https://github.com/facebook/react/tree/master/src/renderers/dom. The latter structure avoids deeply nested require calls such as require('../../../../../src/path/to/my/module/file.js') but often requires more configuration.
In the case of the former, name tests with an extension of .test.js, this makes it easy to distinguish between src/test in the command palette in most editors.
Try to avoid utility packages and instead name things better or consider whether the function/module is granular enough if you cannot name it easily. Try to avoid generic names such as handler, processor or loader if possible.
Use complete names for things, avoid abbreviations such as req, res and err. Although these are terser, they're less readable.
Don't use chai functions in production code, if you want to use assert, use the builtins: https://nodejs.org/dist/latest-v4.x/docs/api/assert.html. Chai is designed for readability in a test environment, I have no idea if it's production ready and I don't believe it has ever been advertised as such.
Prefer using a functional style, leaning on tools such as lodash or Ramda to supplement missing tools in the standard ECMAscript library.
Prefer using map/filter/reduce to for (var item in array) or for (var i; i++; i < n) loops. These are chainable and require less boilerplate than their counterparts. They also allow you to extract the work and pass in the function directly.
Try to avoid forEach unless necessary as it will likely involve more nesting and auxillary objects/arrays than it's other counterparts listed above.
Reduce is very useful for looping over objects and mutating them using Object.keys (or Object.entries / Object.values in newer versions of ECMAscript).
Use Object.assign (docs) whereever possible, preferrably with an empty object as the first argument (Object.assign({}, source, newValue) as this returns a new object without modifying the original (there are some caveats here as the clone operation is only shallow).
This is a minefield and it's a mistake to address it in one brief summary. Origami.ft.com seems to provide a detailed guide to writing good FE code, but I'll list some problems which should be addressed.
- Place script tags at the bottom of the body
- Identify browser support. Avoid using jQuery unless < IE9 is a requirement. youmightnotneedjquery.com offers a list of alternatives.
- Avoid using logic in templates as much as possible
- Consider using a CSS naming scheme such as BEM to manage styles. Extact CSS into a separate heirarchy and use a pre or post processor (sass/postcss). Alternatively consider CSS-in-JS but this is a much newer paradigm
- Bundle scripts separately to templates and include them via CDN
- Avoid attaching handlers directly to DOM elements as this tightly couples your DOM, JS and load order.
There may be some team skillset/cultural/reasonable reasons to implement this all server side, but I'd personally have seriously considered writing the front end as a single-page-app using a modern web framework such as 'react', as I have had nothing but bad experiences maintaining and testing FE code which is comprised of vanillaJS/jQuery and disparate page or component handlers, especially when doing complex form interactions.
Add unit tests for unit testable components (queryBuilder screams test me).
Name tests so as to tell the user what the test is doing. Don't just assert the condition, write human readable result.
Try to only have one assertion per test, this makes failures more granular and should assist debugging.
Do not directly assert HTML. Black box the render process, and just check template name + parameters. It's impossible to work out what's gone wrong based on a comparison of the HTML output and makes the tests useless except as a boolean check, they do not help investigation at all. If there's no other option, use a HTML AST parser such as cheerio but be aware that any attempt to do this will lead to brittle tests and vague failures.
Try to use hexagonal architecture (aka ports and adapters) pattern by pushing 3rd party services and HTML/http responses to the edge of the application, then stubbing it in tests to get reproducable output.
Try to avoid talking to databases directly in test unless necessary as it's slow and requires additional setup.