Work in progress. Loose collection of practices to review with developers so we can agree on UI developer guidelines.
We use a tool called ng lint in the Angular client to keep our code readable, consistently formatted, and to avoid some common bugs. Under the hood, ng lint uses a tool called eslint – the eslint website is a good place to find documentation about the lint rules.
You should run ng lint early and often in the development process, as it can help you avoid certain bugs. Definitely run it before submitting your code for community review, or before committing any Angular code to the Evergreen repository.
Ng lint does not require a running Evergreen system. You just need to install a few dependencies before running it for the first time.
npm installnpm install -g @angular/cli@^15 (replace 15 with the version of Angular that Evergreen currently uses, which you can find in the file Open-ILS/src/extras/install/Makefile.common).ng lintnpx ng lintng lint any time it sees a commit to the eg2 directory.ng lint can automatically fix many issues for you. To do this, run ng lint –fixng lint to ignore a certain line if needed. For example, if you want to ignore the error "rxjs/no-implicit-any-catch", you could add a comment before that line:// eslint-disable-next-line no-var var myVariable = 1;
It's a good practice to also say a bit more in this line about why we can't do the version lint would prefer, for example:
// eslint-disable-next-line eqeqeq -- we have to use ==, rather than ===, here since result could be a string or an integer if (result == '2') {
no-magic-numbers: A magic number is a number that you use in your code without explaining what it represents. To resolve this error, rather than using your number directly (e.g. 86400), first assign it to a variable with a descriptive name (e.g. const oneDayInSeconds = 86400), and use the variable instead.no-shadow: You have two variables with the same name in two different scopes (for example, you have a variable named data in your method, and then you go into a loop that uses a different variable also named data). You can resolve this by renaming one (or both) of the variables to something more descriptive.no-cond-assign: Ng lint will complain if you do something like if (myValue = 3), because it's very likely that you meant to compare myValue and 3 (if (myValue === 3)), rather than set the value of myValue to 3. If that's what you meant, you can simply add the extra equal signs. If you do indeed want to assign myValue, consider if it might be possible or clearer to assign it before the conditional, or if you can use parentheses to make it clearer that you are both assigning and comparing in one step.rxjs/no-nested-subscribe: The angular client uses RxJS Observables frequently, especially when fetching data from the server. You may sometimes wish to fetch some data from the server, then, depending on what is in the data, fetch some more. For example, if you want to find some items, set them to deleted, then log their call number to the console, it can be tempting to write something like this:// This code contains 2 rxjs/no-nested-subscribe errors
const idsToDelete = [3, 12, 30];
this.pcrud.search('acp', {id: idsToDelete}).subscribe(item => {
item.deleted(true);
this.pcrud.autoApply(item).subscribe(deletedItem => {
this.pcrud.search('acn', {id: deletedItem.call_number()}).subscribe(callNumber => {
console.log(`We deleted an item related to call number ${callNumber.label()}`);
});
});
});
Note that there are three `subscribe()` calls, nested inside of each other, each of which will probably handle three events (for each of the ids we want to change). Troubleshooting this complexity can get tricky! Instead, you could refactor the above code to use RxJS switchmap, which is built for accepting data from one observable, then switching to a different observable that uses the same data.
// This code has no rxjs/no-nested-subscribe errors
const idsToDelete = [3, 12, 30];
this.pcrud.search('acp', {id: idsToDelete}).pipe(
switchMap(item => {
item.deleted(true);
return this.pcrud.autoApply(item);
}),
switchMap(deletedItem => {
return this.pcrud.search('acn', {id: deletedItem.call_number()});
})
).subscribe(callNumber => {
console.log(`We deleted an item related to call number ${callNumber.label()}`);
});
In this code, the distinction between the different calls and their sequence of events is a bit clearer at a glance, making for easier troubleshooting.