-
-
Notifications
You must be signed in to change notification settings - Fork 621
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 4 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 |
---|---|---|
@@ -0,0 +1,19 @@ | ||
"use strict"; | ||
|
||
const fs = require("fs"); | ||
const path = require("path"); | ||
|
||
/** | ||
* Attempts to detect whether the string is a local path regardless of its | ||
* existence by checking its format. The point is to distinguish between | ||
* paths and modules on the npm registry. This will fail for non-existent | ||
* local Windows paths that begin with a drive letter, e.g. C:..\generator.js, | ||
* but will succeed for any existing files and any absolute paths. | ||
* | ||
* @param {String} str - string to check | ||
* @returns {Boolean} whether the string could be a path to a local file or directory | ||
*/ | ||
|
||
module.exports = function(str) { | ||
return ((path.isAbsolute(str) || /^\./.test(str)) || fs.existsSync(str)); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
"use strict"; | ||
|
||
const isLocalPath = require("./is-local-path"); | ||
const path = require("path"); | ||
|
||
describe("is-local-path", () => { | ||
it("returns true for paths beginning in the current directory", () => { | ||
const p = path.resolve(".", "test", "dir"); | ||
expect(isLocalPath(p)).toBe(true); | ||
}); | ||
|
||
it("returns true for absolute paths", () => { | ||
const p = path.join("/", "test", "dir"); | ||
expect(isLocalPath(p)).toBe(true); | ||
}); | ||
|
||
it("returns false for npm packages names", () => { | ||
expect(isLocalPath("webpack-addons-ylvis")).toBe(false); | ||
}); | ||
|
||
it("returns false for scoped npm package names", () => { | ||
expect(isLocalPath("@webpack/test")).toBe(false); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
"use strict"; | ||
const chalk = require("chalk"); | ||
const isLocalPath = require("./is-local-path"); | ||
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 | ||
if (isLocalPath(addon)) { | ||
// If the addon is a path to a local folder, no name validation is necessary. | ||
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") { | ||
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,26 @@ | ||
const npmPackagesExists = require("./npm-packages-exists"); | ||
|
||
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", () => { | ||
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", () => { | ||
expect(() => npmPackagesExists(["my-webpack-addon"])).toThrowError(TypeError); | ||
}); | ||
|
||
test("resolves packages when they are available on npm", done => { | ||
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")); | ||
}); | ||
}); |
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.
If you're passing the relative path you'll get
../webpack-cli/lib/generators/generator.js isn't a valid name.
andIt 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 pathThere 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 think the
return
statement on line 28 means you'll never make it to that error code.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 got there :(
![skjermbilde 2018-03-03 kl 02 44 14](https://user-images.githubusercontent.com/16735925/36929098-cea0cb58-1e8c-11e8-91c7-a59c80d3da45.png)
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.
Ah OK I see. I will fix it.
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.
Sweet!
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.
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?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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
both 👍