Adds a validator module to Bubblewrap #134
Conversation
- Adds a validator module that validates PWAs for the Trusted Web Activity quality criteria. - The module uses the PageSpeed Insights API to run and collect Lighthouse scoring information. - Adds a `validate` command to `@bubblewrap/cli`.
- Cleans PSI results type for things we really need. - Fixes typo in README.md - validate produces more useful details - build runs PWA Quality Checks - a --skipPwaValidation flag was added to build
- Validation during the build step is now a warning. - Checks for the response status on the PSI request and throws an error if it's not 200. - Addresses review notes.
| - **[bubblewrap/cli](./packages/cli):** a command-line version of Bubblewrap. | ||
| - **[bubblewrap/validator](./packages/validator):** library to validate the correctness and |
There was a problem hiding this comment.
tiniest nit: could you say "a library to...", so that the sentence has the same form as the previous two bullet points?
packages/cli/src/index.ts
Outdated
| .then((result) => { | ||
| if (!result) { | ||
| process.exit(1); | ||
| } | ||
| }) | ||
| .catch((err: Error) => { | ||
| log.error(err.message); | ||
| process.exit(1); |
There was a problem hiding this comment.
I'm not quite sure what you're doing here. Isn't this the last code to be called in the execution of the program? Why would you need to call process.exit() ? Is it just to make the return code 1?
I'm dubious about using process.exit in general since it's not obvious from the caller of this method that the entire program could terminate. Is there some way to make the method return the exit code - so 1 in the two cases you call process.exit in and 0 otherwise?
There was a problem hiding this comment.
cli/src/index.ts is the entry point for the CLI application - a "main" method (TypeScript doesn't have a main). The reason it calls process.exit(1) is to signal an abnormal termination. In case the program is being invoked as part of a build pipeline, this signals that the CLI failed and the whole pipeline can be aborted.
There was a problem hiding this comment.
Could we instead do:
const success = await cli.run(args)
.then((result) => {
return result;
})
.catch((error) => {
// ...
return false;
});
process.exit(success ? 0 : 1);
- Adds a comment to explain why process.exit(1) is used. - Moves implementation to an async method instead of Promises and cleans-up the code.
Activity quality criteria.
Lighthouse scoring information.
validatecommand to@bubblewrap/cli.