-
-
Notifications
You must be signed in to change notification settings - Fork 597
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 all 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 |
---|---|---|
@@ -0,0 +1,36 @@ | ||
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); | ||
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.
Okay lgtm for now 👍