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: pass argument to main function #5

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
2 changes: 1 addition & 1 deletion .npmignore
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
Expand Up @@ -3,10 +3,10 @@
const path = require("path");
const fs = require("mz/fs");
const linter = require("addons-linter");
const withTmpDir = require("./helpers/tmp-dir");
const cmdRunner = require("./helpers/cmd-runner");
const withTmpDir = require("../helpers/tmp-dir");
const cmdRunner = require("../helpers/cmd-runner");

const execDirPath = path.join(__dirname, "..", "bin");
const execDirPath = path.join(__dirname, "../..", "bin");
Copy link
Owner

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)


describe("main", () => {
test("creates files including manifest with correct name", () => withTmpDir(
Expand Down
23 changes: 23 additions & 0 deletions __tests__/unit/errors.test.js
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/);
});
});
});
127 changes: 127 additions & 0 deletions __tests__/unit/index.test.js
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 =
Copy link
Owner

Choose a reason for hiding this comment

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

@saintsebastian we could probably export the getProjectCreatedMessage from the index.js module and use it to generate the expected message here and make this test less verbose (and not affected by changes to the message)

Then, we can test the exported getProjectCreatedMessage on its own (possibly by using expect.stringMatching or expect(...).toMatch(...))

`${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();
Copy link
Owner

Choose a reason for hiding this comment

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

I think that it would be better to assert expect(statRes.isFile()).toBeTruthy() (e.g. because by not being a directory we are also including symbolic links, char and block devices, socket files etc.)


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");
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we raising an exception from this helper?
I would have expected that this test would just try to run the comment when the project directory exists, am I reading it wrong?

});

try {
await main({
dirPath: projName,
baseDir: tmpPath,
getPlaceholderIconFn: getPlaceholderIconMock,
});
} catch (error) {
expect(error.message).toMatch(/error/);
Copy link
Owner

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 onlyInstanceOf.

}
})
);
});
23 changes: 22 additions & 1 deletion bin/create-webextension
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) => {
Copy link
Owner

Choose a reason for hiding this comment

The 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);
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
});
12 changes: 12 additions & 0 deletions errors.js
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 {};
59 changes: 34 additions & 25 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -36,16 +35,17 @@ a WebExtension from the command line:
${chalk.bold.blue("web-ext run -s /path/to/extension")}
`;

const asciiLogo = fs.readFileSync(
Copy link
Owner

Choose a reason for hiding this comment

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

uhm... I'm particularly not thrilled about adding an fs.readFileSync call here, especially considering about this module used as a library, it basically means that the nodejs app that includes this module is going to "pause everything else" until it has loaded the asciiLogo.

is exporting the asciiLogo actually needed? we can discuss about why we need it and find another solution for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 __dirname is different for tests, but now I just pass correct directory.

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) {
Expand Down Expand Up @@ -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,
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need all this options here?
I think we should evaluate these options and decide if it makes sense for an app that includes this module as a library to redefine this helpers or if we need them only for testing reasons.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 = {
Copy link
Owner

Choose a reason for hiding this comment

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

there should be no need to turn the export into a single module.exports assignment, we should be able to export them one by one with exports.main = main, export.asciiLogo = ... etc.

main,
MORE_INFO_MSG,
asciiLogo,
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"homepage": "https://github.com/rpl/create-webextension#readme",
"dependencies": {
"chalk": "^1.1.3",
"es6-error": "^4.0.2",
"mz": "^2.6.0",
"strip-ansi": "^3.0.1"
},
Expand Down