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

misc(generator): Allow local paths to generators #265

Merged
merged 5 commits into from
Mar 8, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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
7 changes: 6 additions & 1 deletion SCAFFOLDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ Before writing a `webpack-cli` scaffold, think about what you're trying to achie

## webpack-addons-yourpackage

In order for `webpack-cli` to compile your package, it relies on a prefix of `webpack-addons`. The package must also be published on npm. If you are curious about how you can create your very own `addon`, please read [How do I compose a webpack-addon?](https://github.com/ev1stensberg/webpack-addons-demo).
In order for `webpack-cli` to compile your package, it must be available on npm or on your local filesystem. If you are curious about how you can create your very own `addon`, please read [How do I compose a
webpack-addon?](https://github.com/ev1stensberg/webpack-addons-demo).

If the package is on npm, its name must have a prefix of `webpack-addons`.

If the package is on your local filesystem, it can be named whatever you want. Pass the name of the package as a relative path to its root directory.
Copy link
Member

Choose a reason for hiding this comment

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

i just noticed that you've removed [How do I compose a webpack-addon?](https://github.com/ev1stensberg/webpack-addons-demo). I would put it back was a good reference to me some time ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll restore that.

Copy link
Member

Choose a reason for hiding this comment

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

Pass the path to the package


## API

Expand Down
25 changes: 19 additions & 6 deletions lib/utils/npm-packages-exists.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"use strict";
const chalk = require("chalk");
const fs = require("fs");
const npmExists = require("./npm-exists");
const resolvePackages = require("./resolve-packages").resolvePackages;

Expand All @@ -14,8 +15,22 @@ const resolvePackages = require("./resolve-packages").resolvePackages;

module.exports = function npmPackagesExists(pkg) {
let acceptedPackages = [];

function resolvePackagesIfReady() {
if (acceptedPackages.length === pkg.length)
return resolvePackages(acceptedPackages);
}

pkg.forEach(addon => {
//eslint-disable-next-line
// The addon is a path to a local folder; no validation is necessary
if (fs.existsSync(addon)) {
acceptedPackages.push(addon);
resolvePackagesIfReady();
return;
}

// The addon is on npm; validate name and existence
// eslint-disable-next-line
if (addon.length <= 14 || addon.slice(0, 14) !== "webpack-addons") {
Copy link
Member

Choose a reason for hiding this comment

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

If you're passing the relative path you'll get ../webpack-cli/lib/generators/generator.js isn't a valid name. and It should be prefixed with 'webpack-addons', but have different suffix.. Can you do so it says ../webpack-cli/lib/generators/generator.js isn't a valid name or path. ? And we shouldn't throw the addons error if it's a path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the return statement on line 28 means you'll never make it to that error code.

Copy link
Member

Choose a reason for hiding this comment

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

I got there :(
skjermbilde 2018-03-03 kl 02 44 14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah OK I see. I will fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait—it looks like you ran the command pointing at the generator js file. Maybe I misunderstood the requirements. Do we want to allow people to point the init command at a javascript file, an npm package, or both?

Copy link
Contributor Author

@dylanonelson dylanonelson Mar 3, 2018

Choose a reason for hiding this comment

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

Ugh, I can't reproduce the error you're seeing. Not sure what the deal is. When I pass a relative path to a generator on my filesystem it resolves it without throwing the prefix error. I'm going to be out of town till next Wednesday, so I won't be able to investigate further till then.

(Also, I realize my above comment re packages vs files is probably beside the point, apologies 😬.)

Copy link
Member

Choose a reason for hiding this comment

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

both 👍

throw new TypeError(
chalk.bold(`${addon} isn't a valid name.\n`) +
Expand All @@ -24,11 +39,12 @@ module.exports = function npmPackagesExists(pkg) {
)
);
}

npmExists(addon)
.then(moduleExists => {
if (!moduleExists) {
Copy link
Member

Choose a reason for hiding this comment

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

This would never hit if we're using local paths, change the error throwing on prefix

Error.stackTraceLimit = 0;
throw new TypeError("Package isn't registered on npm.");
throw new TypeError(`Cannot resolve location of package ${addon}.`);
}
if (moduleExists) {
acceptedPackages.push(addon);
Expand All @@ -38,9 +54,6 @@ module.exports = function npmPackagesExists(pkg) {
console.error(err.stack || err);
process.exit(0);
})
.then(_ => {
if (acceptedPackages.length === pkg.length)
return resolvePackages(acceptedPackages);
});
.then(resolvePackagesIfReady);
});
};
31 changes: 31 additions & 0 deletions lib/utils/npm-packages-exists.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const fs = require("fs");
const npmPackagesExists = require("./npm-packages-exists");

jest.mock("fs");
jest.mock("./npm-exists");
jest.mock("./resolve-packages");

const mockResolvePackages = require("./resolve-packages").resolvePackages;

describe("npmPackagesExists", () => {
test("resolves packages when they are available on the local filesystem", () => {
fs.existsSync.mockReturnValueOnce(true);
npmPackagesExists(["./testpkg"]);
expect(mockResolvePackages.mock.calls[mockResolvePackages.mock.calls.length - 1][0]).toEqual(["./testpkg"]);
});

test("throws a TypeError when an npm package name doesn't include the prefix", () => {
fs.existsSync.mockReturnValueOnce(false);
expect(() => npmPackagesExists(["my-webpack-addon"])).toThrowError(TypeError);
});

test("resolves packages when they are available on npm", done => {
fs.existsSync.mockReturnValueOnce(false);
require("./npm-exists").mockImplementation(() => Promise.resolve(true));
npmPackagesExists(["webpack-addons-foobar"]);
setTimeout(() => {
expect(mockResolvePackages.mock.calls[mockResolvePackages.mock.calls.length - 1][0]).toEqual(["webpack-addons-foobar"]);
done();
}, 10);
});
});
30 changes: 29 additions & 1 deletion lib/utils/package-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ function spawnYarn(pkg, isNew) {
*/

function spawnChild(pkg) {
const pkgPath = path.resolve(globalPath, pkg);
const rootPath = getPathToGlobalPackages();
const pkgPath = path.resolve(rootPath, pkg);
Copy link
Member

Choose a reason for hiding this comment

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

Okay lgtm for now 👍

const packageManager = getPackageManager();
const isNew = !fs.existsSync(pkgPath);

Expand All @@ -71,7 +72,34 @@ function getPackageManager() {
return "yarn";
}

/**
*
* Returns the path to globally installed
* npm packages, depending on the available
* package manager determined by `getPackageManager`
*
* @returns {String} path - Path to global node_modules folder
*/
function getPathToGlobalPackages() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed? We already have a package manager function that takes care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which function are you referring to? I don't see any code in pacakge-manager.js that serves the same purpose.

const manager = getPackageManager();

if (manager === "yarn") {
try {
const yarnDir = spawn
.sync("yarn", ["global", "dir"])
.stdout.toString()
.trim();
return path.join(yarnDir, "node_modules");
} catch (e) {
// Default to the global npm path below
}
}

return globalPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

could we put the require of this into the function? So that in the future no one mixes maybe globalPath with getPathToGlobalPackage().

const globalPath = require("global-modules");
const manager = getPackageManager();

Then the scope of the function is completely independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

}

module.exports = {
getPackageManager,
getPathToGlobalPackages,
spawnChild
};
32 changes: 30 additions & 2 deletions lib/utils/package-manager.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"use strict";

const path = require("path");

jest.mock("cross-spawn");
jest.mock("fs");

Expand Down Expand Up @@ -27,6 +29,11 @@ describe("package-manager", () => {
);
}

function mockSpawnErrorTwice() {
mockSpawnErrorOnce();
mockSpawnErrorOnce();
}

spawn.sync.mockReturnValue(defaultSyncResult);

it("should return 'yarn' from getPackageManager if it's installed", () => {
Expand Down Expand Up @@ -65,7 +72,7 @@ describe("package-manager", () => {
it("should spawn npm install from spawnChild", () => {
const packageName = "some-pkg";

mockSpawnErrorOnce();
mockSpawnErrorTwice();
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask, what the reason for having same spawn calls twice instead of one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spawnChild now calls spawn.sync twice, because it calls getPackageManager twice. It calls getPackageManager once and also calls getPathToGlobalPackages. which in turn calls getPackageManager. I can refactor it if that seems too redundant.

packageManager.spawnChild(packageName);
expect(spawn.sync).toHaveBeenLastCalledWith(
"npm",
Expand All @@ -77,7 +84,7 @@ describe("package-manager", () => {
it("should spawn npm update from spawnChild", () => {
const packageName = "some-pkg";

mockSpawnErrorOnce();
mockSpawnErrorTwice();
fs.existsSync.mockReturnValueOnce(true);

packageManager.spawnChild(packageName);
Expand All @@ -87,4 +94,25 @@ describe("package-manager", () => {
{ stdio: "inherit" }
);
});

it("should return the yarn global dir from getPathToGlobalPackages if yarn is installed", () => {
const yarnDir = "/Users/test/.config/yarn/global";
// Mock confirmation that yarn is installed
spawn.sync.mockReturnValueOnce(defaultSyncResult);
// Mock stdout of `yarn global dir`
spawn.sync.mockReturnValueOnce({
stdout: {
toString: () => `${yarnDir}\n`
}
});
const globalPath = packageManager.getPathToGlobalPackages();
const expected = path.join(yarnDir, "node_modules");
expect(globalPath).toBe(expected);
});

it("should return the npm global dir from getPathToGlobalPackages if yarn is not installed", () => {
mockSpawnErrorOnce();
const globalPath = packageManager.getPathToGlobalPackages();
expect(globalPath).toBe(require("global-modules"));
});
});
37 changes: 31 additions & 6 deletions lib/utils/resolve-packages.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
"use strict";

const fs = require("fs");
const path = require("path");
const chalk = require("chalk");
const globalPath = require("global-modules");

const creator = require("../init/index").creator;

const getPathToGlobalPackages = require("./package-manager")
.getPathToGlobalPackages;
const spawnChild = require("./package-manager").spawnChild;

/**
Expand Down Expand Up @@ -41,10 +43,36 @@ function resolvePackages(pkg) {

let packageLocations = [];

function invokeGeneratorIfReady() {
if (packageLocations.length === pkg.length)
return creator(packageLocations);
}

pkg.forEach(addon => {
// Resolve paths to modules on local filesystem
if (fs.existsSync(addon)) {
let absolutePath = addon;

try {
absolutePath = path.resolve(process.cwd(), addon);
require.resolve(absolutePath);
packageLocations.push(absolutePath);
} catch (err) {
console.log(`Cannot find a valid npm module at ${absolutePath}.`);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this was asked before, why we dont throw when provided module wasnt found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point—I think that's just a mistake on my part. I will use the code in the catch below to log the error and exit the process if we can't resolve the package.

console.log("\nError:\n");
console.error(chalk.bold.red(err));
process.exitCode = 1;
}

invokeGeneratorIfReady();
return;
}

// Resolve modules on npm registry
processPromise(spawnChild(addon))
.then(_ => {
try {
const globalPath = getPathToGlobalPackages();
packageLocations.push(path.resolve(globalPath, addon));
} catch (err) {
console.log("Package wasn't validated correctly..");
Expand All @@ -55,15 +83,12 @@ function resolvePackages(pkg) {
}
})
.catch(err => {
console.log("Package Couldn't be installed, aborting..");
console.log("Package couldn't be installed, aborting..");
console.log("\nReason: \n");
console.error(chalk.bold.red(err));
process.exitCode = 1;
})
.then(_ => {
if (packageLocations.length === pkg.length)
return creator(packageLocations);
});
.then(invokeGeneratorIfReady);
});
}

Expand Down