Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: overhauling error handling to support the new api error responses #218

Merged
merged 2 commits into from
Jul 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions cmds/docs/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const config = require('config');
const fs = require('fs');
const editor = require('editor');
const { promisify } = require('util');
const APIError = require('../../lib/apiError');

const writeFile = promisify(fs.writeFile);
const readFile = promisify(fs.readFile);
Expand Down Expand Up @@ -61,13 +62,7 @@ exports.run = async function (opts) {
json: true,
...options,
})
.catch(err => {
if (err.statusCode === 404) {
return Promise.reject(err.error);
}

return Promise.reject(err);
});
.catch(err => Promise.reject(new APIError(err)));

await writeFile(filename, existingDoc.body);

Expand All @@ -88,13 +83,7 @@ exports.run = async function (opts) {
await unlink(filename);
return resolve();
})
.catch(err => {
if (err.statusCode === 400) {
return reject(err.error);
}

return reject(err);
});
.catch(err => reject(new APIError(err)));
});
});
};
21 changes: 8 additions & 13 deletions cmds/docs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const config = require('config');
const crypto = require('crypto');
const frontMatter = require('gray-matter');
const { promisify } = require('util');
const APIError = require('../../lib/apiError');

const readFile = promisify(fs.readFile);

Expand Down Expand Up @@ -49,6 +50,9 @@ exports.run = function (opts) {
}

const files = fs.readdirSync(folder).filter(file => file.endsWith('.md') || file.endsWith('.markdown'));
if (files.length === 0) {
return Promise.reject(new Error(`We were unable to locate Markdown files in ${folder}.`));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this error message because if a user tries to use rdme docs with a directory that has subdirectories containing Markdown, we currently can't handle those Markdown files as rdme docs can't handle category creation and management right now.

https://app.asana.com/0/1178132843162889/1183431246035890/f

}

const options = {
auth: { user: key },
Expand All @@ -57,14 +61,6 @@ exports.run = function (opts) {
},
};

function validationErrors(err) {
if (err.statusCode === 400) {
return Promise.reject(err.error);
}

return Promise.reject(err);
}

function createDoc(slug, file, hash, err) {
if (err.statusCode !== 404) return Promise.reject(err.error);

Expand All @@ -73,13 +69,14 @@ exports.run = function (opts) {
json: { slug, body: file.content, ...file.data, lastUpdatedHash: hash },
...options,
})
.catch(validationErrors);
.catch(err => Promise.reject(new APIError(err)));
}

function updateDoc(slug, file, hash, existingDoc) {
if (hash === existingDoc.lastUpdatedHash) {
return `\`${slug}\` not updated. No changes.`;
}

return request
.put(`${config.host}/api/v1/docs/${slug}`, {
json: Object.assign(existingDoc, {
Expand All @@ -89,7 +86,7 @@ exports.run = function (opts) {
}),
...options,
})
.catch(validationErrors);
.catch(err => Promise.reject(new APIError(err)));
}

return Promise.all(
Expand All @@ -106,9 +103,7 @@ exports.run = function (opts) {
...options,
})
.then(updateDoc.bind(null, slug, matter, hash), createDoc.bind(null, slug, matter, hash))
.catch(err => {
return Promise.reject(err);
});
.catch(err => Promise.reject(new APIError(err)));
})
);
};
11 changes: 2 additions & 9 deletions cmds/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const { validate: isEmail } = require('isemail');
const { promisify } = require('util');
const read = promisify(require('read'));
const configStore = require('../lib/configstore');
const APIError = require('../lib/apiError');

const testing = process.env.NODE_ENV === 'testing';

Expand Down Expand Up @@ -53,14 +54,6 @@ exports.run = async function (opts) {
return Promise.reject(new Error('You must provide a valid email address.'));
}

function badRequest(err) {
if (err.statusCode === 400) {
return Promise.reject(err.error);
}

return Promise.reject(err);
}

return request
.post(`${config.host}/api/v1/login`, {
json: { email, password, project, token },
Expand All @@ -72,5 +65,5 @@ exports.run = async function (opts) {

return `Successfully logged in as ${email.green} to the ${project.blue} project.`;
})
.catch(badRequest);
.catch(err => Promise.reject(new APIError(err)));
};
7 changes: 4 additions & 3 deletions cmds/swagger.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const path = require('path');
const config = require('config');
const { prompt } = require('enquirer');
const promptOpts = require('../lib/prompts');
const APIError = require('../lib/apiError');

exports.command = 'swagger';
exports.usage = 'swagger [file] [options]';
Expand Down Expand Up @@ -82,7 +83,7 @@ exports.run = async function (opts) {
function error(err) {
try {
const parsedError = JSON.parse(err.error);
return Promise.reject(new Error(parsedError.description || parsedError.error));
return Promise.reject(new APIError(parsedError));
} catch (e) {
return Promise.reject(new Error('There was an error uploading!'));
}
Expand Down Expand Up @@ -162,8 +163,8 @@ exports.run = async function (opts) {
await request.post(`${config.host}/api/v1/version`, options);

return newVersion;
} catch (e) {
return Promise.reject(e.error);
} catch (err) {
return Promise.reject(new APIError(err));
}
}

Expand Down
13 changes: 3 additions & 10 deletions cmds/versions/create.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const config = require('config');
const semver = require('semver');
const { prompt } = require('enquirer');
const promptOpts = require('../../lib/prompts');
const APIError = require('../../lib/apiError');

exports.command = 'versions:create';
exports.usage = 'versions:create --version=<version> [options]';
Expand Down Expand Up @@ -69,7 +70,7 @@ exports.run = async function (opts) {
json: true,
auth: { user: key },
})
.catch(e => Promise.reject(e.error));
.catch(err => Promise.reject(new APIError(err)));
}

const versionPrompt = promptOpts.createVersionPrompt(versionList || [{}], {
Expand All @@ -93,13 +94,5 @@ exports.run = async function (opts) {
return request
.post(`${config.host}/api/v1/version`, options)
.then(() => Promise.resolve(`Version ${version} created successfully.`))
.catch(err => {
return Promise.reject(
new Error(
err.error && err.error.description
? err.error.description
: 'Failed to create a new version using your specified parameters.'
)
);
});
.catch(err => Promise.reject(new APIError(err)));
};
8 changes: 3 additions & 5 deletions cmds/versions/delete.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const request = require('request-promise-native');
const config = require('config');
const semver = require('semver');
const APIError = require('../../lib/apiError');

exports.command = 'versions:delete';
exports.usage = 'versions:delete --version=<version> [options]';
Expand Down Expand Up @@ -37,12 +38,9 @@ exports.run = async function (opts) {

return request
.delete(`${config.host}/api/v1/version/${version}`, {
json: true,
auth: { user: key },
})
.then(() => Promise.resolve(`Version ${version} deleted successfully.`))
.catch(err => {
return Promise.reject(
new Error(err.error && err.error.description ? err.error.description : 'Failed to delete target version.')
);
});
.catch(err => Promise.reject(new APIError(err)));
};
4 changes: 3 additions & 1 deletion cmds/versions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const request = require('request-promise-native');
const Table = require('cli-table');
const config = require('config');
const versionsCreate = require('./create');
const APIError = require('../../lib/apiError');

exports.command = 'versions';
exports.usage = 'versions [options]';
Expand Down Expand Up @@ -116,5 +117,6 @@ exports.run = function (opts) {
}

return Promise.resolve(getVersionFormatted(versions[0]));
});
})
.catch(err => Promise.reject(new APIError(err)));
};
6 changes: 4 additions & 2 deletions cmds/versions/update.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const config = require('config');
const semver = require('semver');
const { prompt } = require('enquirer');
const promptOpts = require('../../lib/prompts');
const APIError = require('../../lib/apiError');

exports.command = 'versions:update';
exports.usage = 'versions:update --version=<version> [options]';
Expand Down Expand Up @@ -61,7 +62,7 @@ exports.run = async function (opts) {
json: true,
auth: { user: key },
})
.catch(e => Promise.reject(e.error));
.catch(err => Promise.reject(new APIError(err)));

const promptResponse = await prompt(promptOpts.createVersionPrompt([{}], opts, foundVersion));
const options = {
Expand All @@ -78,5 +79,6 @@ exports.run = async function (opts) {

return request
.put(`${config.host}/api/v1/version/${version}`, options)
.then(() => Promise.resolve(`Version ${version} updated successfully.`));
.then(() => Promise.resolve(`Version ${version} updated successfully.`))
.catch(err => Promise.reject(new APIError(err)));
};
35 changes: 35 additions & 0 deletions lib/apiError.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module.exports = class extends Error {
constructor(res) {
let err;

// Special handling to for fetch `res` arguments where `res.error` will contain our API error response.
if (typeof res === 'object') {
if ('error' in res && typeof res.error === 'object') {
err = res.error;
} else {
err = res;
}
} else {
err = res;
}

super(err);

if (typeof err === 'object') {
this.code = err.error;

// If we returned help info in the API, show it otherwise don't render out multiple empty lines as we sometimes
// throw `Error('non-api custom error message')` instances and catch them with this class.
if ('help' in err) {
this.message = [err.message, '', err.help].join('\n');
} else {
this.message = err.message;
}

this.name = 'APIError';
} else {
this.code = err;
this.message = err;
}
}
};
26 changes: 3 additions & 23 deletions rdme.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,37 +8,17 @@ require('./cli')(process.argv.slice(2))
})
.catch(e => {
if (e) {
// `err.message` is from locally thrown Error objects
// `err.error` is from remote API errors
const err = e;

// If we've got a remote API error, extract its contents so we can show the user the error.
if (typeof err.error === 'object' && Object.keys(err.error).length === 3) {
err.message = err.error.error;
err.description = err.error.description;
err.errors = err.error.errors;
}

if (!err.description && !err.errors && err.error) {
if ('message' in err) {
console.error(err.message.red);
} else {
console.error(
`Yikes, something went wrong! Please try again and if the problem persists, get in touch with our support team at ${
`[email protected]`.underline
}.\n`.red
);
}

if (err.message && (typeof err.statusCode === 'undefined' || err.statusCode !== 404))
console.error(err.message.red);

if (err.description) console.error(err.description.red);
if (err.errors) {
const errors = Object.keys(err.errors);

console.error(`\nCause${(errors.length > 1 && 's') || ''}:`.red.bold);
errors.forEach(error => {
console.error(` · ${error}: ${err.errors[error]}`.red);
});
}
}

return process.exit(1);
Expand Down
26 changes: 19 additions & 7 deletions test/cmds/docs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,10 @@ describe('rdme docs', () => {
.get(`/api/v1/docs/${slug}`)
.basicAuth({ user: key })
.reply(404, {
description: 'No doc found with that slug',
error: 'Not Found',
error: 'DOC_NOTFOUND',
message: `The doc with the slug '${slug}' couldn't be found`,
suggestion: '...a suggestion to resolve the issue...',
help: 'If you need help, email [email protected] and mention log "fake-metrics-uuid".',
});

const postMock = nock(config.host, {
Expand Down Expand Up @@ -211,12 +213,17 @@ describe('rdme docs:edit', () => {

const getMock = nock(config.host)
.get(`/api/v1/docs/${slug}`)
.reply(404, { error: 'Not Found', description: 'No doc found with that slug' });
.reply(404, {
error: 'DOC_NOTFOUND',
message: `The doc with the slug '${slug}' couldn't be found`,
suggestion: '...a suggestion to resolve the issue...',
help: 'If you need help, email [email protected] and mention log "fake-metrics-uuid".',
});

return docsEdit.run({ slug, key, version: '1.0.0' }).catch(err => {
getMock.done();
expect(err.error).toBe('Not Found');
expect(err.description).toBe('No doc found with that slug');
expect(err.code).toBe('DOC_NOTFOUND');
expect(err.message).toContain("The doc with the slug 'no-such-doc' couldn't be found");
});
});

Expand All @@ -226,14 +233,19 @@ describe('rdme docs:edit', () => {

const getMock = nock(config.host).get(`/api/v1/docs/${slug}`).reply(200, {});

const putMock = nock(config.host).put(`/api/v1/docs/${slug}`).reply(400, { error: 'Bad Request' });
const putMock = nock(config.host).put(`/api/v1/docs/${slug}`).reply(400, {
error: 'DOC_INVALID',
message: "We couldn't save this doc ({error})",
suggestion: '...a suggestion to resolve the issue...',
help: 'If you need help, email [email protected] and mention log "fake-metrics-uuid".',
});

function mockEditor(filename, cb) {
return cb(0);
}

return docsEdit.run({ slug, key, version: '1.0.0', mockEditor }).catch(err => {
expect(err.error).toBe('Bad Request');
expect(err.code).toBe('DOC_INVALID');
getMock.done();
putMock.done();
expect(fs.existsSync(`${slug}.md`)).toBe(true);
Expand Down
Loading