Remove NodeGit dependency / add slate dependency #11
Conversation
- remove nodegit - refactor to only slate for now
| }, { | ||
| type: 'confirm', | ||
| name: 'initGit', | ||
| message: 'Will you be tracking this theme in git?' |
There was a problem hiding this comment.
Actually, I feel like we should still do this -- even though we don't want to force the user to have git / ssh installed, we want to make it easy to init a git package if they do.
There was a problem hiding this comment.
Sure, I'll have to add NodeGit back as a dependency but the API wouldn't rely on the ssh setup for this anyways.
|
super minor feedback, will 🎩 shortly, but generally lgtm. |
| } | ||
| }); | ||
|
|
||
| if (this.fs.exists(scaffold + '/.npmignore')) { |
There was a problem hiding this comment.
@m-ux I just want to call out this spot... Not sure if this is the best approach but I noticed that npm seems to convert our gitignore to an npmignore on download? Kinda weird...
There was a problem hiding this comment.
yeah this is fine, given the specific bug we're hitting.
Generally speaking everyone suggests to avoid fs.exists and there's a bunch of literature on why it was deprecated, but I still find valid uses for it on occasion (it might just be that I don't understand the problem well enough to quickly see a better alternative though).
There was a problem hiding this comment.
(both you & I have already used fs.exists though in a couple places so NBD :)
|
mostly LGTM, probably remove the question about the scaffold you want to start with and 🚀 |
|
@macdonaldr93, do you know what authentication protocol does NPM use to fetch private repos? In my experience, NPM still uses SSH. |
|
In this case for Slate, it would clone the repo using git. I'm not exactly sure what the fallbacks would be if that fails. https://docs.npmjs.com/cli/install
|
|
Does this new approach work when SSH is not configured? Trying to figure out what it's doing differently from NodeGit. |
|
I don't think it would be that different. The other two reasons we opted for this was that:
|
@Shopify/themes-fed
This is a bit of a crazy refactor. I might have gone a little bit overkill but... my goal was to simplify the process for the first rounds of testing and then we can extend slate cli from there.
I've removed NodeGit as a dependency along with two other packages (bluebird and rimraf). NodeGit requires the end user to setup ssh-agent. If this isn't setup correctly, it can lead to some pretty nasty debugging.
Our alternative approach is for Slate to become a dependency of slate-cli. This change comes with a significant amount of pros.
node_modules.The only dependency on NodeGit other than cloning the Slate repo was to initialize git in the newly created theme (
slate new theme <theme-name>). For the time being, I removed this feature as I think this might be an over-complication... If a dev wants to track their theme in git, they can easily rungit initand get up and running.Lastly, I would like to update Slate's name in
package.jsontoshopify-slatebefore it gets published. I'll make a separate PR for that in Slate.Random error: can't rebuild
npm shrinkwrapdue to npm/npm#13327.