Skip to content
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

Windows CLI #159

Merged
merged 2 commits into from
Mar 1, 2016
Merged

Windows CLI #159

merged 2 commits into from
Mar 1, 2016

Conversation

NogsMPLS
Copy link
Contributor

This PR attempts to fix the whole cli issue over at #80

With these changes I was able to execute the gatsby new docs-site gh:gatsbyjs/gatsby-starter-documentation command without having to rename a bin file.

One note: in order to run gatsby develop I had to go into the docs-site folder and add "use strict"; to the app.js file. After that it all ran fine. Ideally this wouldn't be needed, but can't think of a good alternative right now.

This was tested on a Windows 7 machine running Node 5.7.0 and npm 3.6.0

I would advise testing this branch on mac as well, just to make sure it is still cross compatible. Changes don't seem like they will break anything, but with this cross platform stuff, you never know.

NogsMPLS added a commit to NogsMPLS/gatsby-starter-documentation that referenced this pull request Feb 27, 2016
@NogsMPLS
Copy link
Contributor Author

oh. right. didn't do a lint run before PRing. One minute....

@NogsMPLS
Copy link
Contributor Author

The eslint config in the starter repo doesn't like "use strict" so there is still some work to do around that, unless we want to attach an eslint override on it.

@KyleAMathews
Copy link
Contributor

Why the need for the "use strict" in the first place?
On Sat, Feb 27, 2016 at 10:02 AM Nathan Smith [email protected]
wrote:

The eslint config in the starter repo doesn't like "use strict" so there
is still some work to do around that, unless we want to attach an eslint
override on it.


Reply to this email directly or view it on GitHub
#159 (comment).

@NogsMPLS
Copy link
Contributor Author

usage of let/const, etc.

C:\dev\docs-site\app.js:2
  let context = require.context('./pages', true)
  ^^^

SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode

Might be something with babel I can mess with though

@NogsMPLS
Copy link
Contributor Author

okay, switched up the babel register options and it seems to handle all three commands. 🎉

@KyleAMathews
Copy link
Contributor

I'm a bit confused about that error and your fix... was that an eslint error or was that a node.js error? If I run npm run lint in the starter-blog for example, it lints app.js just fine.

@NogsMPLS
Copy link
Contributor Author

Commit 7c7bf5f was just lint stuff related to the babel-register options I added to gatsby-new/gatsby-develop/gatsby-build files inside of bin. Double quote/single quote and dangling comma stuff.

Commit ab83d36 was to change the babel-register option from being exclusive 'only' to instead be more inclusive for everything and ignore everything in node_modules except gatsby.

@NogsMPLS
Copy link
Contributor Author

oh wait, unless you were referring to gatsbyjs/gatsby-starter-documentation#3

that lint error came from trying to use "use strict" inside app.js.

With commit ab83d36 in this repo, that PR in the starter repo is no longer needed, and so I closed it.

@KyleAMathews
Copy link
Contributor

Yeah I don't get ab83d36 as Gatsby is actually distributed as already compiled by Babel so it shouldn't need this. Also app.js isn't part of Gatsby (it's part of the site) so I don't understand why changing the babel register option would affect an error in app.js.

@NogsMPLS
Copy link
Contributor Author

hmmmm...that would explain some stuff.

For testing I was just doing npm install -g my fork of gatsby, but that fork didnt have the build files, so that would make sense.

im out right now, but ill remove those babel-register options and push the commit up when i get home.

@NogsMPLS
Copy link
Contributor Author

NogsMPLS commented Mar 1, 2016

Maybe a good idea for me to close this branch/PR and squash these commits into 1 since all those changes ended up being reverted and really its just a simple file name change. Agree/Disagree?

@KyleAMathews
Copy link
Contributor

+1 though I don't think you need to close the PR to squash. Just squash and
do a force push.

On Mon, Feb 29, 2016 at 9:29 PM Nathan Smith [email protected]
wrote:

Maybe a good idea for me to close this branch/PR and squash these commits
into 1 since all those changes ended up being reverted and really its just
a simple file name change. Agree/Disagree?


Reply to this email directly or view it on GitHub
#159 (comment).

@NogsMPLS
Copy link
Contributor Author

NogsMPLS commented Mar 1, 2016

yeah, just wasn't sure how you'd feel about a push -f ;P

I'll do that right now.

KyleAMathews added a commit that referenced this pull request Mar 1, 2016
@KyleAMathews KyleAMathews merged commit 17be1b4 into gatsbyjs:master Mar 1, 2016
@KyleAMathews
Copy link
Contributor

This is awesome @NogsMPLS! Turned out to be a pretty simple fix, weird. Whatevers, stay weird Windows :) Going to merge my other PRs real quick and make a new release so hopefully Windows folks will be happy now.

@NogsMPLS
Copy link
Contributor Author

NogsMPLS commented Mar 1, 2016

🎉 🎶 🎺 🎶 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants