-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
Changes from 2 commits
4b5a3ee
19b7953
7a8c02d
0b346b1
3d15ddb
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 |
---|---|---|
|
@@ -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. | ||
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. Pass the path to the package |
||
|
||
## API | ||
|
||
|
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; | ||
|
||
|
@@ -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") { | ||
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. If you're passing the relative path you'll get 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 the 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 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. Ah OK I see. I will fix 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. Sweet! 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. 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 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. 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 😬.) 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. both 👍 |
||
throw new TypeError( | ||
chalk.bold(`${addon} isn't a valid name.\n`) + | ||
|
@@ -24,11 +39,12 @@ module.exports = function npmPackagesExists(pkg) { | |
) | ||
); | ||
} | ||
|
||
npmExists(addon) | ||
.then(moduleExists => { | ||
if (!moduleExists) { | ||
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. 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); | ||
|
@@ -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); | ||
}); | ||
}; |
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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
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. Okay lgtm for now 👍 |
||
const packageManager = getPackageManager(); | ||
const isNew = !fs.existsSync(pkgPath); | ||
|
||
|
@@ -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() { | ||
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. Is this really needed? We already have a package manager function that takes care of this? 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. Which function are you referring to? I don't see any code in |
||
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; | ||
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. could we put the require of this into the function? So that in the future no one mixes maybe
Then the scope of the function is completely independent. 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. Sure! |
||
} | ||
|
||
module.exports = { | ||
getPackageManager, | ||
getPathToGlobalPackages, | ||
spawnChild | ||
}; |
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"); | ||
|
||
|
@@ -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", () => { | ||
|
@@ -65,7 +72,7 @@ describe("package-manager", () => { | |
it("should spawn npm install from spawnChild", () => { | ||
const packageName = "some-pkg"; | ||
|
||
mockSpawnErrorOnce(); | ||
mockSpawnErrorTwice(); | ||
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. May I ask, what the reason for having same spawn calls twice instead of one ? 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.
|
||
packageManager.spawnChild(packageName); | ||
expect(spawn.sync).toHaveBeenLastCalledWith( | ||
"npm", | ||
|
@@ -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); | ||
|
@@ -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")); | ||
}); | ||
}); |
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; | ||
|
||
/** | ||
|
@@ -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}.`); | ||
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. Sorry if this was asked before, why we dont 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. Good point—I think that's just a mistake on my part. I will use the code in the |
||
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.."); | ||
|
@@ -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); | ||
}); | ||
} | ||
|
||
|
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.
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.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.
Good catch, I'll restore that.