-
Notifications
You must be signed in to change notification settings - Fork 6
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: pass argument to main function #5
base: master
Are you sure you want to change the base?
Changes from 8 commits
b2cab46
12be9ae
2e41028
63916af
9b9e16c
1457b19
441432b
1f936d1
599396d
2f52f91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
node_modules/ | ||
assets/screenshot.png | ||
assets/screenshot.png |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
"use strict"; | ||
|
||
const UsageError = require("../../errors").UsageError; | ||
const onlyInstancesOf = require("../../errors").onlyInstancesOf; | ||
|
||
describe("onlyInstancesOf", () => { | ||
test("catches specified error", () => { | ||
return Promise.reject(new UsageError("fake usage error")) | ||
.catch(onlyInstancesOf(UsageError, (error) => { | ||
expect(error).toBeInstanceOf(UsageError); | ||
})); | ||
}); | ||
|
||
test("throws other errors", () => { | ||
return Promise.reject(new Error("fake error")) | ||
.catch(onlyInstancesOf(UsageError, () => { | ||
throw new Error("Unexpectedly caught the wrong error"); | ||
})) | ||
.catch((error) => { | ||
expect(error.message).toMatch(/fake error/); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
"use strict"; | ||
|
||
const path = require("path"); | ||
const chalk = require("chalk"); | ||
const fs = require("mz/fs"); | ||
const withTmpDir = require("../helpers/tmp-dir"); | ||
const onlyInstancesOf = require("../../errors").onlyInstancesOf; | ||
const UsageError = require("../../errors").UsageError; | ||
const main = require("../../index").main; | ||
const MORE_INFO_MSG = require("../../index").MORE_INFO_MSG; | ||
const asciiLogo = require("../../index").asciiLogo; | ||
|
||
describe("main", () => { | ||
test("returns project path and creation message", () => withTmpDir( | ||
async (tmpPath) => { | ||
const projName = "target"; | ||
const targetDir = path.join(tmpPath, projName); | ||
const expectedMessage = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @saintsebastian we could probably export the Then, we can test the exported |
||
`${asciiLogo} \n | ||
Congratulations!!! A new WebExtension has been created at: | ||
|
||
${chalk.bold(chalk.green(targetDir))} ${MORE_INFO_MSG}`; | ||
|
||
const result = await main({ | ||
dirPath: projName, | ||
baseDir: tmpPath, | ||
}); | ||
expect(result.projectPath).toEqual(targetDir); | ||
expect(result.projectCreatedMessage).toEqual(expectedMessage); | ||
}) | ||
); | ||
|
||
test("creates files, readme and manifest with correct name", () => withTmpDir( | ||
async (tmpPath) => { | ||
const projName = "target"; | ||
const targetDir = path.join(tmpPath, projName); | ||
|
||
await main({ | ||
dirPath: projName, | ||
baseDir: tmpPath, | ||
}); | ||
|
||
const contentStat = await fs.stat(path.join(targetDir, "content.js")); | ||
expect(contentStat.isDirectory()).toBeFalsy(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it would be better to assert |
||
|
||
const bgStat = await fs.stat(path.join(targetDir, "background.js")); | ||
expect(bgStat.isDirectory()).toBeFalsy(); | ||
|
||
const rmStat = await fs.stat(path.join(targetDir, "README.md")); | ||
expect(rmStat.isDirectory()).toBeFalsy(); | ||
|
||
const manifest = await fs.readFile(path.join(targetDir, "manifest.json"), "utf-8"); | ||
const parsed = JSON.parse(manifest); | ||
expect(parsed.name).toEqual(projName); | ||
}) | ||
); | ||
|
||
test("calls all of its necessary dependencies", () => withTmpDir( | ||
async (tmpPath) => { | ||
const projName = "target"; | ||
const targetDir = path.join(tmpPath, projName); | ||
|
||
const getProjectManifestMock = jest.fn(); | ||
const getPlaceholderIconMock = jest.fn(); | ||
const getProjectReadmeMock = jest.fn(); | ||
const getProjectCreatedMessageMock = jest.fn(); | ||
|
||
await main({ | ||
dirPath: projName, | ||
baseDir: tmpPath, | ||
getProjectManifestFn: getProjectManifestMock, | ||
getPlaceholderIconFn: getPlaceholderIconMock, | ||
getProjectReadmeFn: getProjectReadmeMock, | ||
getProjectCreatedMessageFn: getProjectCreatedMessageMock, | ||
}); | ||
|
||
expect(getProjectManifestMock.mock.calls.length).toBe(1); | ||
expect(getProjectManifestMock.mock.calls[0][0]).toBe(projName); | ||
|
||
expect(getPlaceholderIconMock.mock.calls.length).toBe(1); | ||
|
||
expect(getProjectReadmeMock.mock.calls.length).toBe(1); | ||
expect(getProjectReadmeMock.mock.calls[0][0]).toBe(projName); | ||
|
||
expect(getProjectCreatedMessageMock.mock.calls.length).toBe(1); | ||
expect(getProjectCreatedMessageMock.mock.calls[0][0]).toBe(targetDir); | ||
}) | ||
); | ||
|
||
test("throws Usage Error when directory already exists", () => withTmpDir( | ||
async (tmpPath) => { | ||
const projName = "target"; | ||
const targetDir = path.join(tmpPath, projName); | ||
await fs.mkdir(targetDir); | ||
|
||
try { | ||
await main({ | ||
dirPath: projName, | ||
baseDir: tmpPath, | ||
}); | ||
} catch (error) { | ||
onlyInstancesOf(UsageError, () => { | ||
expect(error.message).toMatch(/dir already exists/); | ||
}); | ||
} | ||
}) | ||
); | ||
|
||
test("throws error when directory already exists", () => withTmpDir( | ||
async (tmpPath) => { | ||
const projName = "target"; | ||
const getPlaceholderIconMock = jest.fn(() => { | ||
throw new Error("error"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we raising an exception from this helper? |
||
}); | ||
|
||
try { | ||
await main({ | ||
dirPath: projName, | ||
baseDir: tmpPath, | ||
getPlaceholderIconFn: getPlaceholderIconMock, | ||
}); | ||
} catch (error) { | ||
expect(error.message).toMatch(/error/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to be sure that this test fails if it does not raise an error when the directory exists (and check that we have received the expected error message). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpl this time i tried to achieve this using jest own methods, take a look maybe there is no need at all to have |
||
} | ||
}) | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,24 @@ | ||
#!/usr/bin/env node | ||
const chalk = require("chalk"); | ||
const UsageError = require("../errors").UsageError; | ||
const onlyInstancesOf = require("../errors").onlyInstancesOf; | ||
|
||
require("..").main(); | ||
|
||
const USAGE_MSG = `Usage: create-webextension project_dir_name`; | ||
|
||
if (!process.argv[2]) { | ||
console.error(`${chalk.red("Missing project dir name.")}\n`); | ||
console.log(USAGE_MSG); | ||
process.exit(1); | ||
} | ||
|
||
require("..").main({dirPath: process.argv[2]}) | ||
.then(({projectCreatedMessage}) => console.log(projectCreatedMessage)) | ||
.catch(onlyInstancesOf(UsageError, (error) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @saintsebastian it would be better to use console.error here (so that the logs are going to be printer in the stderr instead of stdout). also, after this catch we should handle the other errors by chaining another catch: .catch(onlyInstancesOf(UsageError, (error) => {
...
})
.catch((error) => {
console.error(`${chalk.red(error.message)}: ${error.stack}\n`); // or something similar that prints both the message and the stack
process.exit(1);
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpl wanted to avoid chaining to catches, so thanks for making this clear. |
||
console.error(`${chalk.red(error.message)}\n`); | ||
process.exit(1); | ||
})) | ||
.catch((error) => { | ||
console.error(`${chalk.red(error.message)}: ${error.stack}\n`); // or something similar that prints both the message and the stack | ||
process.exit(1); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
const ES6Error = require("es6-error"); | ||
|
||
exports.onlyInstancesOf = function (errorType, handler) { | ||
return (error) => { | ||
if (error instanceof errorType) { | ||
return handler(error); | ||
} | ||
throw error; | ||
}; | ||
}; | ||
|
||
exports.UsageError = class UsageError extends ES6Error {}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,7 @@ const path = require("path"); | |
const chalk = require("chalk"); | ||
const fs = require("mz/fs"); | ||
const stripAnsi = require("strip-ansi"); | ||
|
||
const USAGE_MSG = `Usage: create-webextension project_dir_name`; | ||
const UsageError = require("./errors").UsageError; | ||
|
||
const README = ` | ||
This project contains a blank WebExtension addon, a "white canvas" for your new experiment of | ||
|
@@ -36,16 +35,17 @@ a WebExtension from the command line: | |
${chalk.bold.blue("web-ext run -s /path/to/extension")} | ||
`; | ||
|
||
const asciiLogo = fs.readFileSync( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uhm... I'm particularly not thrilled about adding an is exporting the asciiLogo actually needed? we can discuss about why we need it and find another solution for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpl I agree this is not ideal, i decided to do that because |
||
path.join(__dirname, "assets", "webextension-logo.ascii") | ||
); | ||
|
||
function getProjectCreatedMessage(projectPath) { | ||
return fs.readFile(path.join(__dirname, "assets", "webextension-logo.ascii")) | ||
.then(asciiLogo => { | ||
const PROJECT_CREATED_MSG = `\n | ||
Congratulations!!! A new WebExtension has been created at: | ||
const PROJECT_CREATED_MSG = `\n | ||
Congratulations!!! A new WebExtension has been created at: | ||
|
||
${chalk.bold(chalk.green(projectPath))}`; | ||
|
||
return `${asciiLogo} ${PROJECT_CREATED_MSG} ${MORE_INFO_MSG}`; | ||
}); | ||
return `${asciiLogo} ${PROJECT_CREATED_MSG} ${MORE_INFO_MSG}`; | ||
} | ||
|
||
function getProjectReadme(projectDirName) { | ||
|
@@ -87,39 +87,48 @@ function getProjectManifest(projectDirName) { | |
}; | ||
} | ||
|
||
exports.main = function main() { | ||
if (!process.argv[2]) { | ||
console.error(`${chalk.red("Missing project dir name.")}\n`); | ||
console.log(USAGE_MSG); | ||
process.exit(1); | ||
function main({ | ||
dirPath, | ||
baseDir = process.cwd(), | ||
getProjectManifestFn = getProjectManifest, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need all this options here? If they are only needed for testing reasons, with jest we can probably just move them in another module file and use the Jest Mocks feature to mock them (http://facebook.github.io/jest/docs/manual-mocks.html) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rpl i used auto mocking in this case, I think it acheives what you mean |
||
getPlaceholderIconFn = getPlaceholderIcon, | ||
getProjectReadmeFn = getProjectReadme, | ||
getProjectCreatedMessageFn = getProjectCreatedMessage, | ||
}) { | ||
if (!dirPath) { | ||
throw new Error("Project directory name is a mandatory argument"); | ||
} | ||
|
||
const projectPath = path.resolve(process.argv[2]); | ||
const projectPath = path.resolve(baseDir, dirPath); | ||
const projectDirName = path.basename(projectPath); | ||
|
||
return fs.mkdir(projectPath).then(() => { | ||
return Promise.all([ | ||
fs.writeFile(path.join(projectPath, "manifest.json"), | ||
JSON.stringify(getProjectManifest(projectDirName), null, 2)), | ||
JSON.stringify(getProjectManifestFn(projectDirName), null, 2)), | ||
fs.writeFile(path.join(projectPath, "background.js"), | ||
`console.log("${projectDirName} - background page loaded");`), | ||
fs.writeFile(path.join(projectPath, "content.js"), | ||
`console.log("${projectDirName} - content script loaded");`), | ||
]).then(() => getPlaceholderIcon()) | ||
]).then(() => getPlaceholderIconFn()) | ||
.then(iconData => fs.writeFile(path.join(projectPath, "icon.png"), iconData)) | ||
.then(() => getProjectReadme(projectDirName)) | ||
.then(() => getProjectReadmeFn(projectDirName)) | ||
.then(projectReadme => fs.writeFile(path.join(projectPath, "README.md"), | ||
stripAnsi(projectReadme))) | ||
.then(() => getProjectCreatedMessage(projectPath)) | ||
.then(console.log); | ||
.then(async () => { | ||
const projectCreatedMessage = await getProjectCreatedMessageFn(projectPath); | ||
return {projectPath, projectCreatedMessage}; | ||
}); | ||
}, error => { | ||
if (error.code === "EEXIST") { | ||
const msg = `Unable to create a new WebExtension: ${chalk.bold.underline(projectPath)} dir already exist.`; | ||
console.error(`${chalk.red(msg)}\n`); | ||
process.exit(1); | ||
const msg = `Unable to create a new WebExtension: ${chalk.bold.underline(projectPath)} dir already exists.`; | ||
throw new UsageError(msg); | ||
} | ||
}).catch((error) => { | ||
console.error(error); | ||
process.exit(1); | ||
}); | ||
} | ||
|
||
module.exports = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there should be no need to turn the export into a single |
||
main, | ||
MORE_INFO_MSG, | ||
asciiLogo, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saintsebastian this should be
path.join(__dirname, "..", "..", "bin)
(so that nodejs can add the right path separator based on the current OS)