-
-
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
Conversation
Hi, thanks for the PR! Could you manage to fix the issue you posted as well in this PR? |
Sure thing, will do. Will take me a few days to get to it. |
Gotcha, no worries! |
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! |
@ev1stensberg Any chance you or another maintainer would have time to take a look at this sometime soon? |
@dylanonelson I'll have a look at it later today 👍 |
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.
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 )
lib/utils/npm-packages-exists.js
Outdated
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" |
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.
This need revising if local folders are allowed, so webpack-addons
for npm installed and whatever for local ones.
lib/utils/npm-packages-exists.js
Outdated
chalk.red( | ||
"\nIt should be prefixed with 'webpack-addons', but have different suffix.\n" | ||
) | ||
); | ||
} | ||
npmExists(addon) | ||
|
||
packageExists(addon) | ||
.then(moduleExists => { | ||
if (!moduleExists) { |
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.
This would never hit if we're using local paths, change the error throwing on prefix
lib/utils/package-exists.js
Outdated
* based on if it exists or not | ||
*/ | ||
|
||
module.exports = function packageExists(pkg) { |
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.
👍
* | ||
* @returns {String} path - Path to global node_modules folder | ||
*/ | ||
function getPathToGlobalPackages() { |
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.
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 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.
lib/utils/npm-packages-exists.js
Outdated
@@ -1,6 +1,7 @@ | |||
"use strict"; | |||
const chalk = require("chalk"); | |||
const npmExists = require("./npm-exists"); | |||
const packageExists = require("./package-exists"); | |||
const path = require("path"); |
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.
Path should be above packageExists
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.
maybe its work enabling absolute imports eslint check?
SCAFFOLDING.md
Outdated
@@ -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). |
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.
Change this to describe diff from local packages ( no prefix needed ) and npm ones
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 didn't realize that local packages didn't have to have the prefix.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Okay lgtm for now 👍
lib/utils/package-exists.js
Outdated
*/ | ||
|
||
module.exports = function packageExists(pkg) { | ||
const isValidLocalPath = fs.existsSync(pkg); |
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.
not sure its worth caching into variable as its a sync call
Any update on this? |
This week was busy for me, but I was just going to take another look today. Will push some changes by tomorrow. |
No worries, there's no rats race. Take your time 💙 |
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. |
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 |
88fae6f
to
0f0fe3e
Compare
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
210aee3
to
5798ed6
Compare
I think this is ready for another review, @EugeneHlushko @ev1stensberg. I've added some tests for I should add that the test file for |
SCAFFOLDING.md
Outdated
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Pass the path to the package
lib/utils/npm-packages-exists.js
Outdated
} | ||
|
||
// 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 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
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 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.
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 👍
@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. |
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.
Looks good, thank you so much! 👍 🎉
lib/utils/npm-packages-exists.js
Outdated
@@ -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 ( |
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 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
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.
For sure. Just pushed a commit to refactor that code.
@dylanonelson Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
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.
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?
} | ||
} | ||
|
||
return globalPath; |
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.
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.
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.
Sure!
@@ -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 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 ?
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.
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.
@dylanonelson @fokusferit left some comments, could you open a PR to fix those? Need your changes to another PR |
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
./
or../
.require
ing—i.e., that they have all their dependencies installed. It does not link them globally.