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

dylanonelson · 2018-02-12T03:10:52Z

This is a first attempt at fixing #262. I'd like some feedback on this approach before I add tests and hunt down the corresponding documentation. Thanks!

What kind of change does this PR introduce?
feature

Did you add tests for your changes?
Not yet! Would like some feedback on this approach before I write tests.

If relevant, did you update the documentation?
I removed a line from SCAFFOLDING.md, but there are probably other places where the documentation needs updating.

Summary
Fixes #262. Allows users to pass a relative path to the init command so that they can compose generator packages not published to npm.

Does this PR introduce a breaking change?
No.

Other information

The current implementation treats the package name as a potential relative path by default. It does not validate that the path starts with ./ or ../.
It assumes that the user‘s local generator packages are ready for requireing—i.e., that they have all their dependencies installed. It does not link them globally.

evenstensberg · 2018-02-12T14:25:34Z

Hi, thanks for the PR! Could you manage to fix the issue you posted as well in this PR?

dylanonelson · 2018-02-12T16:08:44Z

Sure thing, will do. Will take me a few days to get to it.

evenstensberg · 2018-02-13T23:43:35Z

Gotcha, no worries!

dylanonelson · 2018-02-14T21:11:53Z

I've updated the pr to fix #266 as well. I know I need to add tests, but since this is my first js pr I'd like feedback on the way it's implemented before I spend the time to set them up. There are some nuances to how the cli validates and resolves package names that you may have different ideas about. Thanks!

dylanonelson · 2018-02-20T18:46:41Z

@ev1stensberg Any chance you or another maintainer would have time to take a look at this sometime soon?

evenstensberg · 2018-02-20T19:50:27Z

@dylanonelson I'll have a look at it later today 👍

evenstensberg

Left some comments. We've introduced a commit linter in master, so make sure that your commits are styled propperly. If you need help with the style: git add. then git cz ( if you've rebased )

evenstensberg · 2018-02-20T19:58:28Z

lib/utils/npm-packages-exists.js

 			throw new TypeError(
-				chalk.bold(`${addon} isn't a valid name.\n`) +
+				chalk.bold(`${addonBasename} isn't a valid name.\n`) +
 					chalk.red(
 						"\nIt should be prefixed with 'webpack-addons', but have different suffix.\n"


This need revising if local folders are allowed, so webpack-addons for npm installed and whatever for local ones.

evenstensberg · 2018-02-20T19:59:20Z

lib/utils/npm-packages-exists.js

 					chalk.red(
 						"\nIt should be prefixed with 'webpack-addons', but have different suffix.\n"
 					)
 			);
 		}
-		npmExists(addon)
+
+		packageExists(addon)
 			.then(moduleExists => {
 				if (!moduleExists) {


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

evenstensberg · 2018-02-20T19:59:32Z

lib/utils/package-exists.js

+ * based on if it exists or not
+ */
+
+module.exports = function packageExists(pkg) {


evenstensberg · 2018-02-20T20:04:17Z

lib/utils/package-manager.js

+ *
+ * @returns {String} path - Path to global node_modules folder
+ */
+function getPathToGlobalPackages() {


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

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

evenstensberg · 2018-02-20T20:04:43Z

lib/utils/npm-packages-exists.js

@@ -1,6 +1,7 @@
 "use strict";
 const chalk = require("chalk");
-const npmExists = require("./npm-exists");
+const packageExists = require("./package-exists");
+const path = require("path");


Path should be above packageExists

maybe its work enabling absolute imports eslint check?

evenstensberg · 2018-02-20T20:05:26Z

SCAFFOLDING.md

@@ -14,7 +14,7 @@ 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 relies on a prefix of `webpack-addons`. 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).


Change this to describe diff from local packages ( no prefix needed ) and npm ones

Ah OK, I didn't realize that local packages didn't have to have the prefix.

evenstensberg · 2018-02-20T20:06:12Z

lib/utils/package-manager.js

@@ -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);


Okay lgtm for now 👍

EugeneHlushko · 2018-02-21T13:27:17Z

lib/utils/package-exists.js

+ */
+
+module.exports = function packageExists(pkg) {
+	const isValidLocalPath = fs.existsSync(pkg);


not sure its worth caching into variable as its a sync call

evenstensberg · 2018-02-25T18:49:56Z

Any update on this?

dylanonelson · 2018-02-25T19:07:37Z

This week was busy for me, but I was just going to take another look today. Will push some changes by tomorrow.

evenstensberg · 2018-02-25T21:00:26Z

No worries, there's no rats race. Take your time 💙

dylanonelson · 2018-02-26T03:30:47Z

I'm a little confused by this output because it says the tests are all passing. So where are the errors?

UPDATE: I see now. The commit-lint is failing. I will rebase and squash the changes into a cz-approved commit.

evenstensberg · 2018-02-26T19:43:27Z

It's your commit format, if you so a biopsy commit using git add followed up by git cz you can see how they're supposed to be

webpack-bot · 2018-02-26T22:24:36Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

dylanonelson · 2018-02-26T22:44:45Z

I think this is ready for another review, @EugeneHlushko @ev1stensberg.

I've added some tests for npm-packages-exists.js and for the function I added in package-manager.js. I ran into issues trying to test resolve-packages.js, because the code I added calls require.resolve, and I couldn't figure out a clean way to mock that function. Some of these tests seem very closely tied to the implementations they're meant to test, but perhaps that's inevitable given the number of IO operations in the code. Would appreciate any feedback on that topic.

I should add that the test file for resolve-packages.js is a bit strange, at least to my eyes—it doesn't actually import resolve-packages.js, so I'm not sure what it's testing.

evenstensberg · 2018-03-02T13:28:31Z

SCAFFOLDING.md

+
+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.


Pass the path to the package

evenstensberg · 2018-03-03T01:35:02Z

lib/utils/npm-packages-exists.js

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


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

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

I got there :(

Ah OK I see. I will fix it.

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?

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

dylanonelson · 2018-03-03T03:26:10Z

@ev1stensberg had an hour tonight to spare. I think my changes address the error you were seeing. If you review in the next few days I can pick it up again next week.

evenstensberg

Looks good, thank you so much! 👍 🎉

ematipico · 2018-02-13T09:06:33Z

lib/utils/npm-packages-exists.js

@@ -20,7 +20,10 @@ module.exports = function npmPackagesExists(pkg) {
 		const addonBasename = path.basename(addon);

 		//eslint-disable-next-line
-		if (addonBasename.length <= 14 || addonBasename.slice(0, 14) !== "webpack-addons") {
+		if (


I see made a small change here. Would you mind to refactor 14 and put inside a variable, please? Numbers without context are hard to read

For sure. Just pushed a commit to refactor that code.

webpack-bot · 2018-03-07T17:46:34Z

@dylanonelson Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

ematipico

We require to change the test files from *.spec.js to *.test.js. As you can see codecov is complaining because it tries to run coverage con the test files. Could you apply this change please?

fokusferit · 2018-03-08T10:55:25Z

lib/utils/package-manager.js

+		}
+	}
+
+	return globalPath;


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.

fokusferit · 2018-03-08T11:05:37Z

lib/utils/package-manager.spec.js

@@ -65,7 +72,7 @@ describe("package-manager", () => {
 	it("should spawn npm install from spawnChild", () => {
 		const packageName = "some-pkg";

-		mockSpawnErrorOnce();
+		mockSpawnErrorTwice();


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

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.

evenstensberg · 2018-03-08T15:55:52Z

@dylanonelson @fokusferit left some comments, could you open a PR to fix those? Need your changes to another PR

webpack-bot added PR: unreviewed PR: CI-ok labels Feb 12, 2018

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Feb 12, 2018

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Feb 14, 2018

evenstensberg reviewed Feb 20, 2018

View reviewed changes

evenstensberg changed the title ~~[WIP] Allow paths to generators in addition to yarn/npm package names~~ misc(generator): Allow local paths to generators Feb 20, 2018

EugeneHlushko reviewed Feb 21, 2018

View reviewed changes

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Feb 26, 2018

dylanonelson force-pushed the allow-init-paths branch 2 times, most recently from 88fae6f to 0f0fe3e Compare February 26, 2018 22:15

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Feb 26, 2018

dylanonelson force-pushed the allow-init-paths branch 2 times, most recently from 210aee3 to 5798ed6 Compare February 26, 2018 22:38

evenstensberg requested changes Mar 3, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 3, 2018

misc(generator): allow local paths to generators

7a8c02d

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Mar 3, 2018

misc(generator): allow local paths to generators

0b346b1

evenstensberg approved these changes Mar 6, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Mar 6, 2018

evenstensberg requested review from ematipico and fokusferit March 6, 2018 15:30

ematipico approved these changes Mar 6, 2018

View reviewed changes

misc(generator): allow local paths to generators

3d15ddb

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Mar 7, 2018

ematipico suggested changes Mar 8, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Mar 8, 2018

fokusferit reviewed Mar 8, 2018

View reviewed changes

evenstensberg merged commit 8011b5a into webpack:master Mar 8, 2018

dylanonelson mentioned this pull request Mar 10, 2018

Fix global path require & add vim swap files to gitignore #332

Merged


		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.

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

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

Conversation

dylanonelson commented Feb 12, 2018

evenstensberg commented Feb 12, 2018

dylanonelson commented Feb 12, 2018

evenstensberg commented Feb 13, 2018

dylanonelson commented Feb 14, 2018

dylanonelson commented Feb 20, 2018

evenstensberg commented Feb 20, 2018

evenstensberg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EugeneHlushko Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

evenstensberg commented Feb 25, 2018

dylanonelson commented Feb 25, 2018

evenstensberg commented Feb 25, 2018

dylanonelson commented Feb 26, 2018 • edited Loading

evenstensberg commented Feb 26, 2018

webpack-bot commented Feb 26, 2018

dylanonelson commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanonelson Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanonelson commented Mar 3, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Mar 7, 2018

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Mar 8, 2018

evenstensberg left a comment •

edited

Loading

EugeneHlushko Feb 21, 2018 •

edited

Loading

dylanonelson commented Feb 26, 2018 •

edited

Loading

dylanonelson Mar 3, 2018 •

edited

Loading