JS Style Guide
Table of Contents
1. Coding Style
2. Documentation
3. Separation of concerns
4. Writing components
5. Writing services
6. Handling tech debt
Coding Style
This best practices guide focuses on design and naming conventions rather than making an exhaustive list of prescriptions. To manage more microscopic aspects of JS and Vue coding style, we lean on our linters. See a breakdown of linting for the Grove repo here.
Documentation
At Grove, we believe that “self-documenting code” is not enough — we should strive to document the right details to make our lives easier. This applies to frontend code too.
Here are some good things to document:
- In general, expected interfaces. For instance,
- API client code should be accompanied by example usage patterns
- Functions should have docstrings specifying the purpose of the function, param types and return type [a la JSDoc](https://jsdoc.app/tags-typedef.html](https://jsdoc.app/tags-typedef.html)
- Major organizational modules should have readme.md’s specifying what belongs in the module (example: https://github.com/groveco/grove-static/blob/master/services/README.md)
- New repos should have readme.md’s describing setup, setup prerequisites, installation, commands to run and so forth.
- Non-obvious details the reader would not be able to figure out on their own. This is often information about why your code exists or is written like it is. For instance,
- describe what edge case is being handled
- shed light on the constraint that led to a counterintuitive decision
Bad:
cartTotal() {
if (
!this.meetsCheckoutMinimum &&
this.stateHandler !== this.getOrderMinimumState
) {
this.stateHandler = this.getOrderMinimumState;
}
if (this.waitForTippers) {
return;
}
this.updateTimeout = setTimeout(() => {
this.refreshState();
}, 750);
},
Good:
cartTotal() {
/*
* Default the state handler to the getOrderMinimumState if they are
* currently below the order minimum.
* This is necessary for when the user reaches the minimum then
* removes some items that places them below the minimum again.
*/
if (
!this.meetsCheckoutMinimum &&
this.stateHandler !== this.getOrderMinimumState
) {
this.stateHandler = this.getOrderMinimumState;
}
if (this.waitForTippers) {
/*
* Avoid refreshing the state if we are still waiting for tipper
* data to be populated. This flag **should** only be enabled
* during the initial mount of the component
*/
return;
}
this.updateTimeout = setTimeout(() => {
this.refreshState();
}
},
Bad:
handleCloseATCDrawer(event) {
const addToCartButtonLabels = [
...SuccessMessagesCollection.toJSON().map(
successMessage => successMessage.message
)
];
if (
!event ||
!addToCartButtonLabels.find(
label =>
event.target.innerText && event.target.innerText.includes(label)
)
) {
this.atcDrawer.isOpen = false;
}
}
Good:
handleCloseATCDrawer(event) {
const addToCartButtonLabels = [
...SuccessMessagesCollection.toJSON().map(
successMessage => successMessage.message
)
];
if (
!event ||
!addToCartButtonLabels.find(
label =>
event.target.innerText && event.target.innerText.includes(label)
)
) {
/* Right now the PDP has product carousels that are written in Marionette and not Vue.
The if condition above can be removed when we convert those carousels to Vue.
Clicking on the Add to Cart button when it is in Vue does not register as an outside click
on the atcDrawer, but when it is a Marionette component it does.
*/
this.atcDrawer.isOpen = false;
}
}
Here’s what you don’t need to document:
- What your code is doing, as long as your code is straightforward or self-evident.
It can be helpful to summarize blocks of code, but beware that implementation details often change with time. Comments describing implementation are liable to get out of date. It is best to err on the side of clear, easy to read code - i.e. code broken up into small functions with descriptive variable and function names.
Bad
const a = 9.81; // gravitational force
const b = 5; // time in seconds
const c = (1/2)*a*(b^2) // multiply the time and gravity together to get displacement.
Good
const computeDisplacement = (timeInSeconds) => {
const gravitationalForce = 9.81;
const displacement = (1 / 2) * gravitationalForce * (timeInSeconds ^ 2);
return displacement;
}
Examples pulled from https://stackoverflow.com/a/209089.
Commenting style
Multi-line comments should use block commenting, e.g.
Bad
// This is a
// multi-line
// comment.
Good
/*
* This is a multi-line comment.
* You can set up your editor to automatically format a comment like this.
*/
For single-line comments, use the double slash.
Bad
/* Using a multi-line comment for a single line. */
Good
// Using a double slash for single line comment.
Separation of concerns
Separation of concerns is a common principle behind several practices in this guide. Separating logic out in terms of responsibility encourages loose coupling and keeps our application modular.
At the application level,
- Components are View Models (VM): they have the responsibility of syncing business models with the view.
- Services handle operational business logic (CRUD).
- Backbone currently handles communication with the server.
- Utils and helpers handle common computational tasks.
- The analytics helper class handle analytics.
And therefore our frontend code has the following layout:
- assets/js
- components
- constants
- services
- utils
- helpers/analytics
More reading on SOC & SRP:
https://stackoverflow.com/a/25012230
https://softwareengineering.stackexchange.com/a/32614
https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html
Writing components
Official Vue guidelines
The first thing to know about component-writing at Grove is to follow official Vue guidelines. If you are new to Vue or could use a refresh, we highly encourage you to read or review each topic in the following docs:
https://vuejs.org/v2/style-guide/
Our linters are currently set to warn (but not block commits or merges) on a violation of the recommended Vue styles (we hope to switch over to erroring on violations soon). To catch violations of these rules, use npm run lint — or better yet, configure your editor to show lint errors as you edit code.
Not all of our codebase adheres to the official Vue style guide, and you may find some well-established conventions in our code run counter to the guide. However, we should strive to ensure that new code, and all existing code that we touch, does conform.
Some common gotcha’s to remember…
- .data as a function
- Detailed prop definitions, including types
- Always use key with v-for
- Never use v-if on the same element as v-for
- Always use kebab-case for event names
… But it’s best to internalize the full list of best practices described in the Vue documentation (Y)
Using the component library
The next thing to consider is whether your component or elements of your component belong in our component library, Skylight. We firmly believe we should use a component library to avoid duplication of code and design discrepancies across pages and applications.
As of 2021/09/03, v1.0 of Grove’s Skylight library is under development and the library is only being used in the grove-static repo.
Therefore, if your component:
- has a use across different pages of the application, or across mobile and web
or
- is composed of smaller design elements seen in other components
make sure to log it in this intake doc that we are using to track development needs for Skylight:
https://docs.google.com/spreadsheets/d/1CdNgC6dG2ZXKwyPyAHre33PFYMP2Ur-NU0S1zQCA5D8/edit#gid=0
Once Skylight is available for general development, this guide will link to the instructions for incorporating Skylight into your development process, including
- developing with existing Skylight components
- proposing new Skylight components
- contributing new Skylight components
- porting existing components into Skylight
Naming of components
Much of this is covered in the Vue style guide, but for convenience is restated here.
- Component names should always be multi-word, except for root App components, and built-in components provided by Vue
- Base components (a.k.a. presentational, dumb, or pure components) that apply app-specific styling and conventions should all begin with a specific prefix (these components also belong to the Skylight component library) - Sky<ComponentName>
- Components that should only ever have a single active instance should begin with the The prefix, to denote that there can be only one.
- Child components that are tightly coupled with their parent should include the parent component name as a prefix.
- Component names should start with the highest-level (often most general) words and end with descriptive modifying words.
- Component names should prefer full words over abbreviations.
Grove-specific naming conventions:
Components that render the main content specific to a route are described as pages (since they dictate the content specific to that page).
e.g.
TheOrderReviewPage
TheCatalogResultsPage
Concerns of the component
Components are VM’s: they have the responsibility of syncing business models with the view.
Things that are concerns of your component:
- Deciding what to present to the user based on business models
- Handling UI events by calling services to conduct business logic
- Deciding what to present to the user based on attempted business logic (i.e., service errors and outcomes)
- Emitting errors to Sentry based on unexpected outcomes of attempted business logic
- Emitting analytics based on UI events (e.g. cta clicks) and how such events are handled (e.g. signup on cta click)
Things that may be a concern of your component:
- If your component is a page component, it should handle any redirects to make.
Things that are not the concern of your component:
- Conducting the business logic itself
- Getting data, serializing data, updating data (components respond to changes in business models, but handling the data itself should be the responsibility of a service)
- Avoid making Backbone calls such as .get or .getRelationship directly in components. See: “Data-fetching services: Return plain JavaScript objects, not Backbone models”
Keeping your component slim / dumb / presentational when possible and pulling business logic out into services is ideal. This is because it makes it easier to quickly read and understand the component, as well as easier to test.
Bad (handles data-fetching directly in the component, rather than using services):
<template>
<div class="OrderSnapshot">
<LoadingIndicator v-if="!lastShipment" class="OrderSnapshot_Loading" />
<LastOrderSnapshot
v-else
:is-expanded="lastShipmentIsExpanded"
:is-primary="!isThreeDaysPastShipped"
:items-are-expanded="lastShipmentItemsAreExpanded"
:shipment-date="lastShipment.get('parsedShipmentDate')"
:shipment-items="lastShipmentItems"
:tracking-link="lastShipment.get('trackingLink')"
:tracking-number="lastShipment.get('trackingNumber')"
@toggle-expanded="toggleLastOrder"
@toggle-expanded-items="toggleLastOrderItems"
@track-link="trackClick($event, null, lastShipment.get('statusName'))"
/>
<LoadingIndicator v-if="!nextShipment" class="OrderSnapshot_Loading" />
<NextOrderSnapshot
v-else
:amount-below-minimum="amountBelowMinimum"
:is-expanded="nextShipmentIsExpanded"
:is-primary="isThreeDaysPastShipped"
:items-are-expanded="nextShipmentItemsAreExpanded"
:items-loading="nextShipmentItemsLoading"
:can-ship="nextShipmentCanShip"
:shipment-date="nextShipmentDate"
:shipment-items-data="nextShipmentItemsData"
@toggle-expanded="toggleNextOrder"
@toggle-expanded-items="toggleNextOrderItems"
@track-link="trackClick($event, null, nextShipment.get('statusName'))"
/>
</div>
</template>
<script>
// External modules
import * as Sentry from '@sentry/vue';
import analytics from '@/helpers/analytics';
import { isAfter, sub } from 'date-fns';
// Components
import LastOrderSnapshot from './LastOrderSnapshot';
import NextOrderSnapshot from './NextOrderSnapshot';
import LoadingIndicator from '@/components/LoadingIndicator';
// Services
import getCustomer from '@/services/get-customer';
import getFirstEditableShipment from '@/services/get-first-editable-shipment';
import getLastShipment from '@/services/get-last-shipment';
export default {
components: {
LastOrderSnapshot,
NextOrderSnapshot,
LoadingIndicator
},
data() {
return {
customer: null,
lastShipment: null,
lastShipmentIsExpanded: false,
lastShipmentItemsAreExpanded: false,
nextShipment: null,
nextShipmentIsExpanded: false,
nextShipmentItemsData: [],
nextShipmentItemsAreExpanded: false,
nextShipmentItemsLoading: true
};
},
computed: {
amountBelowMinimum() {
[...]
},
isThreeDaysPastShipped() {
[...]
},
lastShipmentItems() {
[...]
},
nextShipmentDate() {
[...]
},
nextShipmentCanShip() {
[...]
}
},
created() {
this.fetchCustomer();
this.fetchLastOrder();
this.fetchNextOrder();
},
methods: {
async fetchCustomer() {
this.customer = await getCustomer(this.$store);
},
async fetchLastOrder() {
try {
const shipment = await getLastShipment(this.$store);
if (!shipment) throw shipment;
this.lastShipment = shipment;
} catch (e) {
if (window.boot && window.boot.DEBUG) {
// eslint-disable-next-line no-console
console.debug(e);
}
Sentry.captureException(e);
}
},
async fetchNextOrder() {
try {
const shipment = await getFirstEditableShipment(this.$store);
if (!shipment) throw shipment;
this.nextShipment = shipment;
const items = await shipment.getRelated('items', {
include: 'variant,variant.product',
ordering: 'adjusted_price,-created_at',
insert: 0,
refundedPreorders: 0
});
const itemsPromises = items.map(async item => {
const variant = await item.getRelated('variant');
if (variant.get('isAvailable')) {
const product = await variant.getRelated('product');
const data = {
fullName: variant.get('customerFacingName'),
id: item.id,
name: product.get('name'),
quantity: item.get('quantity')
};
return data;
}
return null;
});
const unfilteredData = await Promise.all(itemsPromises);
this.nextShipmentItemsData = unfilteredData.filter(
data => data !== null
);
this.nextShipmentItemsLoading = false;
} catch (e) {
if (window.boot && window.boot.DEBUG) {
// eslint-disable-next-line no-console
console.debug(e);
}
Sentry.captureException(e);
}
},
toggleLastOrder() {
this.trackClick(
'home_order_snapshot_clicked',
'Last_Order',
this.lastShipment.get('statusName')
);
this.lastShipmentIsExpanded = !this.lastShipmentIsExpanded;
this.nextShipmentIsExpanded = false;
this.nextShipmentItemsAreExpanded = false;
},
toggleLastOrderItems(update) {
this.lastShipmentItemsAreExpanded = update;
if (update) {
this.trackClick(
'home_order_snapshot_expanded_clicked',
'Last_Order',
this.lastShipment.get('statusName')
);
}
},
toggleNextOrder() {
this.trackClick(
'home_order_snapshot_clicked',
'Next_Order',
this.nextShipment.get('statusName')
);
this.nextShipmentIsExpanded = !this.nextShipmentIsExpanded;
this.lastShipmentIsExpanded = false;
this.lastShipmentItemsAreExpanded = false;
},
toggleNextOrderItems(update) {
this.nextShipmentItemsAreExpanded = update;
if (update) {
this.trackClick(
'home_order_snapshot_expanded_clicked',
'Next_Order',
this.nextShipment.get('statusName')
);
}
},
trackClick(action, label, status) {
analytics.structuredEvent('home', action, label, status);
}
}
};
</script>
<style lang="scss">
[ ... ]
</style>
Good:
<template>
<div class="SignupForm">
<form class="SignupForm_Content">
<SkyInput
id="user-email"
v-model="email"
:error="error"
:placeholder="inputPlaceholder"
class="SignupForm_Input"
block
type="email"
>
<template v-if="invalidEmailError" #error>
Hmm - that doesn't seem to be a valid email address.
</template>
<template v-else #error>
You are not eligible for this offer. If you are a current customer,
please <a href="https://www.grove.co/pantry-login">login</a>.
</template>
</SkyInput>
<SkyButton
type="submit"
class="SignupForm_Button"
data-id="acquisition_flow_get_started_welcome_cta"
@click.prevent="onSubmit"
>
</SkyButton>
</form>
<div class="SignupForm_Disclaimer">
By submitting your email, you accept Grove's
<SkyLink :to="ROUTES.privacy.to"></SkyLink> and
<SkyLink :to="ROUTES.terms.to">.</SkyLink>
</div>
</div>
</template>
<script>
import { ROUTES } from '@/constants'
import { SkyInput, SkyButton, SkyLink } from '@groveco/skylight'
import {
registerUser,
CustomerSubmittedInvalidEmail,
CustomerSubmittedIneligibleEmail,
CustomerCheckedOutBefore,
CustomerIsSignedIn,
} from '@/services/register-user'
import { navigateTo } from '@/services/navigate-to'
import { AcquisitionFlow } from '@/analytics/acquisition-flow/events'
export const DEFAULT_INPUT_PLACEHOLDER = 'Enter your email address'
export const DEFAULT_CTA_LABEL = 'Claim My Offer'
export default {
components: {
SkyInput,
SkyLink,
SkyButton,
},
props: {
inputPlaceholder: {
type: String,
default: DEFAULT_INPUT_PLACEHOLDER,
},
ctaLabel: {
type: String,
default: 'Claim My Offer',
},
},
data: () => ({
email: null,
/** Error to surface to user, if any */
error: null,
ROUTES,
RegistrationError: {
CustomerSubmittedIneligibleEmail,
CustomerSubmittedInvalidEmail,
},
}),
computed: {
invalidEmailError() {
return this.error instanceof CustomerSubmittedInvalidEmail
},
},
methods: {
async onSubmit(event) {
this.$analytics.emit(AcquisitionFlow.CTAClicked, {
currentUrl: window.location.href,
ctaId: event.target.getAttribute('data-id'),
})
try {
const newOrUpdatedCustomer = await registerUser(
this.$api,
this.$sentry,
this.email
)
navigateTo(ROUTES.productOfferPicker.to)
this.$analytics.emit(AcquisitionFlow.EmailSignupMade, {
sessionCustomer: newOrUpdatedCustomer,
})
} catch (error) {
const { constructor, sessionCustomer } = error
switch (error.constructor) {
case CustomerSubmittedInvalidEmail:
this.error = error
this.$analytics.emit(AcquisitionFlow.ErrorShown, {
constructor,
sessionCustomer,
})
return
case CustomerSubmittedIneligibleEmail:
this.error = error
this.$analytics.emit(AcquisitionFlow.ErrorShown, {
error,
sessionCustomer,
})
return
case CustomerCheckedOutBefore:
navigateTo(ROUTES.memberHomepage.to)
return
case CustomerIsSignedIn:
navigateTo(ROUTES.productOfferPicker.to)
return
default:
/*
* An unexpected error arose. This may mean that no session/customer
* has been established, and/or the submitted email hasn't been
* registered onto the customer. We do a best effort to proceed
* since it is still be possible for the customer to claim offer and
* sign up through checkout.
*/
if (sessionCustomer?.email !== this.email) {
this.$analytics.emit(AcquisitionFlow.EmailSignupFailed, {
sessionCustomer,
})
}
this.$sentry.captureException(error, {
extra: {
message: 'Unexpected error during customer signup.',
sessionCustomer,
submittedEmail: this.email,
},
})
navigateTo(ROUTES.productOfferPicker.to)
}
}
},
},
}
</script>
<style lang="scss">
[ ... ]
</style>
More reading:
https://vuejs-course.com/blog/separating-ui-and-business-logic-in-vue-components
https://javascript.plainenglish.io/react-vue-and-business-logic-19df105698a2
Communication
Component communication should be fairly intuitive for engineers familiar with Vue standards. Below are a few callouts to the important topics that we want to enforce during development.
Data Down, Events Up
Just as with all traditional Vue development, we should strictly adhere to the data down, events up mindset when it comes to parent-child communication. This means that component props should only consume basic data structures and any kind of component reactivity should be handled by events emitted from children components.
This means that we should avoid passing any kind of callback functions down to children components as callbacks are not data (they are functionality). Instead, engineers should ensure that triggers for logic related to the functionality of a component are defined within the component. We should not force the child to call functionality defined in the parent.
Two-way Data Binding
For components that make use of two-way binding, engineers should ensure that components support the definition of a model. To support v-model binding, components must accept a value prop and emit an input event (using the new state of the value) on changes to the provided value prop. It is important that the data structure of the value is persisted in input events to ensure that the data structure is not changing unexpectedly for the parent component making use of the two-way binding.
Engineers should ensure that user-interactable inputs always make use of the standards proposed by v-model so that we have a common pattern for consuming user input.
Props
As we are working on a unified suite of components, we should be ensuring that components use similar standards for naming props so that engineers can easily intuit use.
Please refer to the Skylight styleguide for instructions on standardized props around presentational logic.
As a side note: if your component features standard props such as disabled, loading, pending, block and readonly, that may be a cue that your component features base presentational logic and belongs in the Skylight repository. If appropriate, log it in this intake doc for Skylight:
https://docs.google.com/spreadsheets/d/1CdNgC6dG2ZXKwyPyAHre33PFYMP2Ur-NU0S1zQCA5D8/edit#gid=0
Other notes on props:
Events
Similar to having to introduce commonality between props, we also need to introduce some standards around how we name events from components.
For all events:
For specific classes of events:
input
For components that capture a single value for user-input or selected data, we should always emit an input event along with the newly updated value. Emitting this specific event assists in supporting the v-model binding discussed in the [two-way data binding section]](#communication)). This event should be used in components similar to <input>, <select>, <textarea>, etc.
click
For components that allow for a single user action without data, emit a click event. This event really pertains to any HTML element and shouldn’t need to be explicitly emitted very often. Most of the time a v-on="$listeners" on the base semantic HTML element is a suitable solution for many semantic elements.
${context}-click
For components that have multiple click actions not involving data, explicitly emit what context emitted the click. For example, if we had a component that displays both a CTA and a close action, we should emit cta-click and close-click respectively.
${context}-change
For components with multiple user-selectable elements, specify a separate event for each action the user can take to mutate a context or model. You may be tempted to create a state object that can be emitted as an input event to support v-model. Doing so forces us to define the state of the component (to avoid mutating child state definitions in the parent) using a single object prop, which is an anti-pattern.
For each value, explicitly emit a separate {context}-change event for that context. For example, if we have a product and variant selector in a single component, we should be emitting a product-change and variant-change respectively on changes. We are specifically not using the name “input” as the suffix of this event to avoid conflation between two-way binding events and contextual change events.
Accessibility
As part of your work, you should be receiving AC around accessibility specified by your designer. If you find that accessibility AC are missing, it’s worth flagging to your team. Not handling accessibility is a form of tech debt.
Accessibility best practices for engineers are described here:
https://docs.google.com/presentation/d/1pRtpxMdsxAkv7g5zdp9sY747a-RPBImTE-gzRLQViJI/edit
and here:
https://groveco.atlassian.net/wiki/spaces/ENGDOC/pages/1704886573/WIP+Accessibility
Accessibility guidelines for specific types of UI are described here:
https://docs.google.com/presentation/d/1a9bkSenhLXMpSkq_jI0-MoVKVXwt09B63qxPtrNMhlc/edit
At Grove, we care about making our UI accessible. Not only that, but failing to make our UI accessible is a form of discrimination and violates the law.
Writing manual test cases
Verifying your changes locally with [dxm](https://github.com/groveco/deus-ex-machina](https://github.com/groveco/deus-ex-machina) or on staging is an important and routine part of the UI development process. For mid-sized features+, there are typically several cases to think about. Your QA engineer may not be aware of all the edge cases or the riskiest scenarios based on the code. If you go through the same steps repeatedly to check certain cases, that’s a good cue to document a manual test suite. Documented test cases are helpful not only for handing off your feature for QA but also in the future for regression testing.
Example of an eng-written test suite:
Writing automated tests with Jest
If you have written your component well (see:Concerns-of-the-component), writing an automated test for your component should be feasible. Ideally, any new component should be accompanied by a test - even the dumbest components benefit from simple render tests.
The Vue testing framework that we use is vue-test-utils and best practices to go with vue-test-utils can be found here: https://vue-test-utils.vuejs.org/guides/common-tips.html.
Mocking data vs functions
There are two types of testing to consider for component tests.
One is an integration test that you can achieve if you mock data and environment only (rather than the functions called by your component). In doing so you test not only the component logic but the interface between your component and the called functions. This allows you to achieve more test coverage and more confidence about your code, but may require painstaking setup of fixtures and maintenance.
The other option is unit testing. By mocking out the functions called by your component, you can focus on what your component does in response to mocked return values and error scenarios. The mocked out functions should be covered by their own tests and it’s worth considering each case for those functions when writing cases for the component.
Writing end-to-end tests with Cypress
Ideally, major UI flows would be covered by end-to-end tests, whether that’s signing up and checkout for the first time or updating contact information. The QA org has taken on some of the responsibility of adding end-to-end tests, but if you see low hanging fruit for Cypress coverage do raise it to your team.
Example of a Cypress test:
https://github.com/groveco/graphitehq/blob/master/cypress/integration/welcome-flow.spec.js
Writing services
Services are focused pieces of reusable and testable code used across the application, with cross-server traffic implied. In other words, services contain logic to interface with the business (make changes to business models or CRUD).
By extracting business logic out of components, we can observe separation of concerns and keep components light and digestible. Moreover, introducing services as a layer of business logic means that we can abstract JsonAPI and Backbone (.get, .getRelationship) considerations out of components. Backbone.Store is our API and data caching layer, originally written at Grove with Marionette in mind - we will want to eventually migrate away from it.
Reusable helper code that does not interface with business data nor make network calls are considered utilities.
Here are some guidelines on writing services:
- All services are promise-based (async-await)
- All services have a single responsibility
- All services throw errors on falsy states rather than returning null, undefined, false, etc.
- This sets up a convention where we can always expect a return value if the service is successful at its task, and expect an unambiguous error otherwise.
- This avoids ending up with several ambiguous none types or a single none type representing more than one falsy case for the return value.
- All services make use of some passed in API client interface
- If a service does not need an API client interface, it is a utility, not a service
- All services are functional in the sense that they do not mutate parameters or global application state
- Services may have business side effects in terms of calling the Django API
- All services operate on builtin types or explicitly defined (typedef’d) objects
- All services are fully documented with appropriate jsdoc docstrings
- All services return early by doing checks and throwing errors before actual function logic is performed
- Prevent all console logging in production; services should not be logging anyway
- Page components (not services) should be concerned with presenting errors once promise-based services throw errors
Bad:
/** Processes offer claim based on presence or absence of sessionid. */
const processOfferClaim = async (email, fallbackOfferCodeParam, formId) => {
const fallbackOfferCode = fallbackOfferCodeParam || null;
const session = await getCurrentSession();
if (session && session.customerId) {
await handleExistingSession(session.customerId, email, fallbackOfferCode);
return;
}
// If there's no preexisting sessionid, we can attempt customer signup.
const endpoint = new URL('/api/customer/', window.location.origin);
const request = {
method: 'post',
headers: vndApiHeaders,
body: JSON.stringify({ data: { type: 'customer', attributes: { email } } }),
};
const response = await fetch(endpoint, request);
const result = await parseResponseBody(response);
try {
if (
(response.status === 201 || response.status === 200) &&
result?.data?.id
) {
const customerId = result?.data?.id;
if (analyticsEnabled) {
// To match behavior in SPA, we log success on customer creation success
// (rather than success for the entire flow).
trackGetStartedSuccess();
const hashedEmail = await hashEmail(email);
if (hashedEmail) {
trackEmailSignupSuccess(customerId, hashedEmail);
}
}
// Make a best-attempt at applying the offer for the customer
// Regardless of failure, we will still kick them into the SPA
let appliedOfferCode = null;
try {
appliedOfferCode = await attemptOfferApplicationWithFallback(
email,
customerId,
fallbackOfferCode
);
} catch (error) {
console.error(error);
}
redirectToGrove('/how-it-works/offers/', appliedOfferCode);
return;
}
if (
response.status === 400 &&
result?.errors?.message.startsWith(
'Account with this email already exists'
)
) {
// customer has checked out before
if (analyticsEnabled) {
trackGetStartedFailure(SIGNUP_STATES.UNAUTH_EXISTING_EMAIL);
}
showSignupError(
'You are not eligible for this offer. If you are a current customer, please <a href="https://www.grove.co/pantry-login">login</a>.',
formId
);
return;
}
throw new UnhandledGroveApiResponseError(
endpoint,
request,
response,
result
);
} catch (error) {
if (analyticsEnabled) {
trackGetStartedFailure(SIGNUP_STATES.UNEXPECTED_ERROR);
}
showSignupError('Oops, something went wrong.', formId);
console.error(error);
if (error instanceof UnhandledGroveApiResponseError) {
console.error('Request object', error.request);
console.error('Response object:', error.response);
console.error('Response data as json:', error.result);
}
}
};
Good:
/**
* Registers a user by associating an email to a customer model.
*
* 1. If no session/customer currently exists: user is registered by creating a
* session and customer with the given email.
* 2. If a session already exists with a customer: user is registered by
* patching the existing customer with the given email.
*
* If the email is not successfully registered onto a customer, an error is
* raised.
*
* Note that if the given email already belongs to a customer, the preexisting
* customer object may get clobbered and the registration may still succeed --
* unless the related customer has already checked out before.
*
* Note that specific errors are raised for when the customer on the session is
* ineligible for registration.
*
* @param {object} api - the API plugin
* @param {object} sentry - the Sentry plugin
* @param {string} email - email to register user with
*
* @throws {CustomerSubmittedIneligibleEmail} When the submitted email already belongs to customer that has checked out
* @throws {CustomerCheckedOutBefore} When customer has checked out before
* @throws {CustomerIsSignedIn} When customer is already signed in
* @throws {UnexpectedError}
*
* @returns {CustomerInfo} The updated session customer
*/
export const registerUser = async (api, sentry, email) => {
if (!validEmail(email)) {
throw new CustomerSubmittedInvalidEmail(undefined)
}
let sessionCustomer
let getError
try {
sessionCustomer = await getSessionCustomer(api, sentry)
} catch (error) {
getError = error
}
if (getError && !(getError instanceof SessionDoesNotExist)) {
throw new Unexpected(getError, undefined)
}
if (getError instanceof SessionDoesNotExist) {
// No active session: initiate session and create new customer
try {
sessionCustomer = await getOrCreateCustomer(api, email)
emit(SessionCustomer.Created, {
customer: sessionCustomer,
})
sentry.configureScope((scope) => {
scope.setUser(sessionCustomer)
})
return sessionCustomer
} catch (error) {
if (error instanceof EmailNotUsable) {
throw new CustomerSubmittedIneligibleEmail(sessionCustomer)
} else {
throw new Unexpected(error, sessionCustomer)
}
}
}
// No error, so session customer exists; patch customer with email if eligible.
if (sessionCustomer.hasCheckedOut) {
throw new CustomerCheckedOutBefore(sessionCustomer)
}
if (sessionCustomer.email) {
throw new CustomerIsSignedIn()
}
try {
sessionCustomer = await updateCustomer(api, sessionCustomer.id, {
email,
})
emit(SessionCustomer.Updated, {
customer: sessionCustomer,
})
sentry.configureScope((scope) => {
scope.setUser(sessionCustomer)
})
return sessionCustomer
} catch (error) {
throw new Unexpected(error, sessionCustomer)
}
}
Best practices for data-fetching
Fetching data with Backbone.Store
Data we retrieve from the Grove API via Grove’s custom Backbone.Store library is returned in the form of a Backbone model or collection, an artifact of our legacy Marionette application which was designed to work with Backbone models.
Determine when you need to fetch data
With Backbone.Store and Grove’s JSON API service, you can fetch an entity’s related data using asynchronous model.getRelated calls. Each of these calls represent another network request if the requested data has not been fetched before during the page visit. Without careful consideration, this can result in dozens of network requests at time of page load.
For example, to display products on the cart or order review page, the client may first get the customer’s pantry, then get the pantry’s related first editable shipment, then get the shipment’s related items, and then for each shipment item, get the related variant, product and brand. For a shipment with 10 shipment items, that can represent 2 + 10 + 10 + 10 + 10 network request (42 requests).
That’s an insane number of network requests! JSON API does provide the option to fetch related data for a resource within a single network request, using include. To use this option, you may specify it during the get or getRelated call:
const variants = await shipment.getRelated(
'variants',
{
query: {
include: 'product,product.brand'
},
}
);
The include option is a double-edge sword, however. Consider that if you get all the shipment item data and their relationships at once (ex: https://www.grove.co/api/pantry/<pantry_id>/first-editable-shipment/?include=items,items.variant,items.variant.product,items.variant.product.brand), the response payload size will inflate and consequently take longer to download and parse. An example request with 10 shipment items can total 144 kb and take 2.08 seconds to finish on a decent internet connection.
Data-fetching services: Return plain JavaScript objects, not Backbone models
As we deprecate usage of Marionette in favor of writing Vue code, we also want to reduce our reliance on Backbone and Backbone.store. To decouple Vue components from the Backbone.store model API and abstract data-fetching away from Backbone.store, please return plain objects from services where possible and rely on services to fetch data from the store.
Bad:
Having multiple components with custom fetching logic with Backbone to fetch brand details. Each of these components would have to be updated in the event that we migrate off Backbone.
Good:
Having each component call services to fetch brand-related data; these services can be updated down the line to use the next API or client storage solution.
<script>
export default {
// [...]
data: () => ({
brand: null,
brandAttributes: null,
brandDetails: null
}),
created() {
this.fetchData();
},
methods: {
async fetchData() {
this.product = await this.$store.get('product', this.productId);
await mapServices(this, {
brand: getBrand(this.product),
brandAttributes: getBrandAttributes(this.product),
brandDetails: getBrandDetails(this.product)
});
}
}
};
</script>
/**
* Get the `brand` of the `product`.
*
* @param {Object} product Product model
* @param {Object} options Options for Backbone Store request
* @returns {Promise|Object} Brand Model
*/
export default async function getBrand(product, options) {
return await product.getRelated('brand', options);
}
/**
* Get the `brandAttributes` related to the `brand`.
*
* @param {Object} product Product model
* @returns {Promise|Object} BrandAttributes Collection
*/
export async function getBrandAttributes(product) {
const brand = await getBrand(product, {
query: {
include: 'attributes'
}
});
return await brand.getRelated('attributes');
}
/**
* Get the customer facing `brandDetails` into a simple Object for easy access and fewer bugs that would break a page.
*
* @param {Object} product Product model
* @param {Object} options Options for Backbone Store request
* @returns {Promise|Object} Customer facing brand attributes
*/
export async function getBrandDetails(product) {
const brand = await getBrand(product);
return brand.pick('name', 'slug', 'description', 'canonicalImage');
}
Handling errors
Some guidelines:
- Let programming errors be thrown from your service as uncaught errors. These should be caught upstream and emitted to Sentry.
- Throw a separate, custom error for each case your service does not handle (outside of programming errors). By specifying error cases, you set up a clear contract for service callers to observe.
- For lower-level errors that you anticipate and need to surface, wrapping with extra context is nice if possible, but tricky to get right.
There’s something like two camps about whether to wrap lower level errors with extra context:
- The first camp advocates for catching and wrapping errors to aid debugging and provide as much context as possible.
- https://openupthecloud.com/error-handling-javascript - “All code that could throw an error should be wrapped in a try/catch with an explicit, detailed error being thrown.”
- https://javascript.info/custom-errors - “Wrapping exceptions is a widespread technique: a function handles low-level exceptions and creates higher-level errors instead of various low-level ones.”
- https://stackoverflow.com/a/26848957 - “Always add some context to the errors (the version, the id of the object, some custom message, …)”
- The second camp argues that error wrapping leads to over-complexity and risks obfuscation.
- https://itnext.io/error-handling-with-async-await-in-js-26c3f20bc06a - “When we create an error on line 2and catch it on 9 , we lose the original stack trace”
- http://disq.us/p/2b1i2fw - “Never add a custom exception class until you know what exact benefit you’ll gain from it.”
If you decide to wrap an error, make sure to provide a good rationale to your team and consider including the original error in the new error to avoid loss of error data.
More reading:
https://stackoverflow.com/a/6852101 - summary of oft-cited talk: “Enterprise JavaScript Error Handling”
https://www.sitepoint.com/proper-error-handling-javascript - event-driven error handling
https://goldbergyoni.com/checklist-best-practices-of-node-js-error-handling/ - a survey of opinions on error handling in JS
Handling analytics
Historically we have used a helper class called analytics to emit events to Sentry in the Grove repo. However, as our data pipelines multiplied over time, this class has grown overloaded and confusing to use, with different track methods emitting to different data platforms. Until we’ve established an alternative, this is still the operating model for adding analytics events in the Grove repo.
Example:
export default async function(store, email) {
const customer = await getCustomer(store);
const formattedCustomerId = formatCustomerId(customer.id);
if (typeof email !== 'string' || email.length <= 0) {
throw new Error('Email must be non-zero length string');
}
customer.set('email', email);
const deferredCustomer = store.update(customer);
try {
await deferredCustomer;
_analytics.structuredEvent(
'customerMembership',
'signupRegistered',
'email'
);
} catch (e) {
let message;
if (e?.response?.responseText) {
const errorBody = JSON.parse(e.response.responseText).errors;
if (e.response.responseText.includes('email already exists')) {
_analytics.acquisitionEvent(
ACQUISITION_FLOW_ACTIONS.SIGNUP_GET_STARTED_FAILED,
formattedCustomerId,
SIGNUP_STATES.UNAUTH_EXISTING_EMAIL
);
} else {
_analytics.acquisitionEvent(
ACQUISITION_FLOW_ACTIONS.SIGNUP_GET_STARTED_FAILED,
formattedCustomerId,
SIGNUP_STATES.UNEXPECTED_ERROR
);
}
message = Object.keys(errorBody)
.map(error => {
if (Array.isArray(errorBody[error])) {
return `${error}: ${errorBody[error].join(',')}`;
} else {
return `${error}: ${errorBody[error]}`;
}
})
.join('\n');
} else {
message = 'Oops, something went wrong';
_analytics.acquisitionEvent(
ACQUISITION_FLOW_ACTIONS.SIGNUP_GET_STARTED_FAILED,
formattedCustomerId,
SIGNUP_STATES.UNEXPECTED_ERROR
);
}
// Undo the customer update
// Fixes GC-1871
customer.set('email', null);
throw new Error(message);
}
const newCustomer = await deferredCustomer;
_analytics.acquisitionEvent(
ACQUISITION_FLOW_ACTIONS.SIGNUP_GET_STARTED_SUCCEEDED,
formattedCustomerId,
SIGNUP_STATES.UNAUTH_NEW_EMAIL
);
_analytics.signup('email', newCustomer);
}
In the grove-static repo we have shifted to using an event-driven paradigm where analytics events are emitted to a Vue event bus, and observers on that event bus handle the decision making of where to emit events.
Example:
async onSubmit(event) {
this.$analytics.emit(AcquisitionFlow.CTAClicked, {
currentUrl: window.location.href,
ctaId: event.target.getAttribute('data-id'),
})
try {
const newOrUpdatedCustomer = await registerUser(
this.$api,
this.$sentry,
this.email
)
navigateTo(ROUTES.productOfferPicker.to)
this.$analytics.emit(AcquisitionFlow.EmailSignupMade, {
sessionCustomer: newOrUpdatedCustomer,
})
} catch (error) {
const { constructor, sessionCustomer } = error
switch (error.constructor) {
case CustomerSubmittedInvalidEmail:
this.error = error
this.$analytics.emit(AcquisitionFlow.ErrorShown, {
constructor,
sessionCustomer,
})
return
case CustomerSubmittedIneligibleEmail:
this.error = error
this.$analytics.emit(AcquisitionFlow.ErrorShown, {
error,
sessionCustomer,
})
return
default:
/*
* An unexpected error arose. This may mean that no session/customer
* has been established, and/or the submitted email hasn't been
* registered onto the customer. We do a best effort to proceed
* since it is still be possible for the customer to claim offer and
* sign up through checkout.
*/
if (sessionCustomer?.email !== this.email) {
this.$analytics.emit(AcquisitionFlow.EmailSignupFailed, {
sessionCustomer,
})
}
this.$sentry.captureException(error, {
extra: {
message: 'Unexpected error during customer signup.',
sessionCustomer,
submittedEmail: this.email,
},
})
navigateTo(ROUTES.productOfferPicker.to)
}
Handling tech debt
As is always the case, as engineers we are likely to come across tech debt over the course of our careers, including when we work on the frontend code. The Frontend Guild exists as a venue to bring up tech debt that is making your life and your teams’ lives harder. We have an intake process for Frontend Guild tickets proposed here. In the meantime, please raise tech debt -related items and questions in the #eng-frontend Slack channel or in our bi-weekly Frontend Guild meetings. Discussion items should be placed in meeting agendas ahead of time.