Starterkit installation fix #5
Conversation
There was an issue (receiving undefined) when trying to install starter kits. This was being caused by not properly getting the npm script variables being passed. Also params variable wasn’t really being used and options variable was never getting set, so this bypasses this check and sets them directly.
| @@ -91,10 +70,10 @@ for (var i=0; i < process.argv.length; i++) { | |||
| liststarterkits(); | |||
| break; | |||
| case 'loadstarterkit': | |||
| loadstarterkit(options.kit, options.clean); | |||
| loadstarterkit(process.env.npm_config_kit, process.env.npm_config_clean); | |||
There was a problem hiding this comment.
@paintedbicycle Where is npm_config_clean being set? Also this was designed for command line passing of the options, and because starterkits and plugins use = and -- to pass options I split them up with the loop above. I tested the installing of a starterkit via command line and it worked with no problem.
There was a problem hiding this comment.
I assumed clean would be set from the command line (--clean) when running the command just like --kit and --plugins.
The documentation describes
yarn patternlab:loadstarterkit --kit=[starterkit-name]
and
--plugin=[plugin-name]
When I set those, both I and the original issue (#3 ) get an "undefined" error. I traced this error back to the original command line config variable "--kit" not being passed properly to this script. When I went digging inside the process.env dump, I found it was being set, but on npm_config_kit and so I changed it.
Now, to be fair, it is --kit that is broken and maybe this fix should be scoped to only that. I assumed that "clean" and "plugins" would be coming from the command line as well since they were all in the same format. The documentation describes plugins to be set in this way, so I believe this is also correct. Clean however I don't see in the docs anywhere in terms of passing it, so maybe it does go back to options.
So, long story short, setting --kit and --plugins in your terminal command was not being passed to this script...and so I've changed it. I first tried keeping the switch and watching the variables being passed, but in terms of plugins and starterkits, this was not happening.
There was a problem hiding this comment.
@paintedbicycle I did test this on Node 6 and not higher, so perhaps that has something to do with it.
There was a problem hiding this comment.
OK, perhaps there was a change in npm after Node v6? Which might mean extra investigating here and potentially a need to have an if statement that targets npm versions.
Let me know how you'd like to proceed. I can create a video and/or further debug on what I'm seeing on this end. I'd imagine we first have to see if this fix breaks older Node versions. Are you targeting a specific set of releases?
There was a problem hiding this comment.
@paintedbicycle I wanted to resolve this issue, your method is much cleaner, and I prefer that over anything else. I will use NVM and swap out the versions to make sure it works with my version and now LTS.
There was a problem hiding this comment.
@paintedbicycle I've taken your suggestion and ran yarn patternlab:loadstarterkit --kit=[starterkit-name] in both 6.10 and 8.9.1 and both return undefined when I log console.log("Starter Kit =", process.env.npm_config_kit);
Can we discuss this outside of these?
There was a problem hiding this comment.
What do you mean by "outside of these"?
There was a problem hiding this comment.
We can move this to gitter to better troubleshoot in the pattern-lab/node group
There was a problem hiding this comment.
Ok I'll sign up.
In the meantime, can you confirm that what you're saying is that if you install my forked repo fresh, and run the following 3 commands, you get an error with node 8.9.1?
npm install
npm install starterkit-mustache-demo
npm run patternlab:loadstarterkit --kit=starterkit-mustache-demo
This works fine for me. But maybe there is an added weirdness with yarn.
|
Made some other updates, but the idea is the same. 1.0.3.2 |
This PR fixes the issue with installing starterkits #3