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

feat(gatsby-cli): add a clean command to wipe out local dirs #9126

Merged
merged 28 commits into from
Feb 19, 2019

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Oct 15, 2018

Fixes #6755

This PR adds a gatsby clean command as a last resort when things have gone awry. We'll need to specifically document use cases when it could make sense to run this, or just emphasize that it's only to be used in certain cases.

Things left to do:

  1. Improve logging (it's fine now)
  2. Integrate prompt for more info
  3. Integrate envinfo for env info
  4. Integrate opt-out/opt-in analytics to gatsby CLI
  5. Store the prompt/envinfo for later investigation
    • I don't know where to put this. Do we have a good pattern for storing data like this?

@DSchau DSchau requested a review from a team as a code owner October 15, 2018 16:50
@DSchau
Copy link
Contributor Author

DSchau commented Oct 16, 2018

@jlengstorf if you have some time, could you check this out and chime in on any of the questions! I think this could be some nice functionality for debugging, helping people, etc.

@DSchau
Copy link
Contributor Author

DSchau commented Oct 17, 2018

Per @pieh perhaps tarball the .cache directory so we can investigate further.

Send this off to a REST API (or something) that we're in control of

Copy link
Contributor

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, @DSchau! Nice work on this. I'm leaving approval to core maintainers. 😅

packages/gatsby-cli/src/index.js Show resolved Hide resolved
### Info
### `clean`

At the root of a Gatsby site run `gatsby clean` to wipe out `node_modules`, cache, and the `public` directory. This is useful as a last resort when your local project seems to have many issues.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would maybe benefit from more of an explanation on why it's a last resort. Are there other things people should try first? Where can they go for more info? How can they help improve the cache so this isn't necessary?

I don't know if we have answers, but it'd be great to include whatever we can here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion!

@KyleAMathews
Copy link
Contributor

Are we adding the snapshot functionality here?

@DSchau
Copy link
Contributor Author

DSchau commented Oct 24, 2018

@KyleAMathews I didn't add it, because I didn't know where it should go, whether we have an API in place, etc. I think the functionality is worth introducing regardless, but it'd be helpful to have some debug info to help us troubleshoot things in the future, so I'm open to releasing it as is or building in some type of API call.

@jlengstorf
Copy link
Contributor

My vote would be to release this and add a follow-up issue to design the API for sending snapshots on clean. Maybe we can add that to the new GraphQL API I just launched? See gatsbyjs/api.gatsbyjs.org#11 for the new code; we could add a mutation in there to accept the snapshot and log it somewhere.

@DSchau
Copy link
Contributor Author

DSchau commented Oct 29, 2018

@pieh you wanna give this a once over and maybe we can get this merged today?

@DSchau DSchau changed the title [WIP]: feat(cli): add a clean command to wipe out local dirs feat(gatsby-cli): add a clean command to wipe out local dirs Nov 13, 2018
@DSchau
Copy link
Contributor Author

DSchau commented Nov 13, 2018

@pieh this is ready now. For those following along, this PR will:

  • Provide a gatsby clean command, which will wipe out public/, .cache, and node_modules
    • This command has arguments, e.g. --no-install, --env-info, etc. which do what their name implies
  • This does not add in any reporting/analysis/snapshot to this command currently, so I'm OK to leave this mostly undocumented for now (README is fine!)
  • Updates the envinfo functionality to default to markdown format

@DSchau
Copy link
Contributor Author

DSchau commented Nov 13, 2018

Going to hold off on merging this until at least tomorrow to iron out some potential issues with Gatsby v1/v2, e.g. the info command not being present in the current version of gatsby that the CLI is being used with and several other issues.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good enough to get this merged, feel free to ignore the latest comments, it shouldn't block the PR to get merged

@DSchau
Copy link
Contributor Author

DSchau commented Feb 7, 2019

@wardpeet want to give this another once over? I scaled it back quite a bit--think it's ready and generally applicable now.

@marcysutton I'll look at docs to see how to best document this feature (troubleshooting?) but let me know if you have any ideas.

@rgiese
Copy link
Contributor

rgiese commented Feb 12, 2019

Just wanted to voice my support for @wardpeet 's comment above re: separating wiping .cache/public from also wiping node_modules. I've often needed to delete .cache (e.g. when messing with webpack config, gatsby-node.js, etc.) but I've never had to delete node_modules.

I think building a command into Gatsby that wipes Gatsby's state (.cache, public) makes a lot of sense. Building a command into Gatsby that wipes someone else's (npm/yarn's) state feels... less clean?

@DSchau
Copy link
Contributor Author

DSchau commented Feb 12, 2019

@rgiese that behavior you describe is exactly how it works :)

I reverted the node_modules deletion in 32a952b

@marcysutton
Copy link
Contributor

@DSchau for docs the "Debugging" section seems like a logical place. But I'm wondering if maybe the section should be renamed "Troubleshooting Gatsby" so we can add things like gatsby clean in there and make it sound less like Gatsby is full of bugs and more about troubleshooting project state. @shannonbux do you have any opinions on a section rename? Or is there a better spot for it? https://www.gatsbyjs.org/docs/debugging/

@shannonbux
Copy link
Contributor

@marcysutton I don't have an opinion; I would just call it whatever seems like the most common / understandable way developers describe the tasks involved in the section. The reason we called it debugging was mostly because other platforms have a "debugging" section, so it is standard (doesn't mean it's the best name, though :)

Here are relevant blog posts about where the current organization of the docs came from:
https://www.gatsbyjs.org/blog/2018-06-26-card-sort-results/
https://www.gatsbyjs.org/blog/2018-07-31-docs-redesign/

@DSchau DSchau requested a review from a team February 13, 2019 22:04
@DSchau DSchau requested a review from a team as a code owner February 13, 2019 22:04
@DSchau
Copy link
Contributor Author

DSchau commented Feb 13, 2019

@shannonbux @marcysutton see f24aa24. Happy to make any changes, I put it together pretty quickly.

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome! One small thing — Shouldn't we exclude node_modules from this?

packages/gatsby/src/commands/clean.js Outdated Show resolved Hide resolved
@sidharthachatterjee sidharthachatterjee merged commit 5807936 into gatsbyjs:master Feb 19, 2019
@sidharthachatterjee
Copy link
Contributor

Successfully published:

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.

10 participants