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

Rename yarn clean #52

Merged
merged 5 commits into from
May 17, 2017
Merged

Rename yarn clean #52

merged 5 commits into from
May 17, 2017

Conversation

jimmytheneutrino
Copy link
Contributor

@bestander
Copy link
Member

Unless there is more evidence from the community that the name is confusing I think it is not worth introducing the breaking change.

@jimmytheneutrino
Copy link
Contributor Author

There are 10 likes and 7 related bugs in the original bug. I haven't searched for more since then, but I am sure there are new ones.

I am now aware of this myself, but somehow managed to shoot myself in the leg two times since then, mistyping yarn clean instead of yarn clean-all or yarn run clean. This breaks sass and the simplest way to fix it was to remove .yarnclean and remove and reinstall node_modules. Something should be done with this.

@bestander
Copy link
Member

Indeed, looks like people do get confused with that feature.
Maybe a better log message could do better at educating the users?
If we end up renaming the command we'll probably need to do it slowly over a few releases with a warning on old syntax.

@pete-otaqui
Copy link

I would agree - yarn clean has the potential (and indeed actual effect) to break third-party modules.

It should have a more descriptive and less general-use name, maybe yarn delete-module-bloat (or something which tells you it's going to delete things).

@jamiebuilds
Copy link
Contributor

I've been affected by this a few times. "clean" is a really common package.json#scripts name and I've often run yarn clean without run because I forgot yarn clean already exists.

@bestander
Copy link
Member

Ok, how does autoclean sound as an alternative name?
We should keep clean command with a warning that it is renamed for a couple of releases

@bestander
Copy link
Member

Looks like no one opposes, @jimmytheneutrino, would you update RFC?

@pete-otaqui
Copy link

pete-otaqui commented May 16, 2017 via email

@dustyjewett
Copy link

I'm curious as to why clean even exists... does this really have an effect?

This absolutely needs to be changed ASAP, and I don't think it matters that the RIGHT name is chosen, because I think everyone can agree that clean is exactly the wrong name.

Suiting it's esoteric nature, I'd recommend something like enable-advanced-auto-disk-space-optimizations... it should ONLY be used by people who are explicitly looking for the solution to a specific problem, and only used once per repository, so I think that a verbose name helps indicate exactly why you would bother typing it all out.

@jamiebuilds
Copy link
Contributor

I think it's silly to want to name it something esoteric. I think autoclean is great, @jimmytheneutrino want to update the RFC?

@L1fescape
Copy link

How about yarn prune?

@jimmytheneutrino
Copy link
Contributor Author

Well, I think that I do not like autoclean after all, because no, it is not "cleaning automatically". I think that prune is great, because https://en.wiktionary.org/wiki/prune#Verb.
But I'd opt for pruneModules, which is even more clear.

@jimmytheneutrino
Copy link
Contributor Author

I updated the rfc with a fixed proposed name. The final name is still to be discussed upon, of course.

@jimmytheneutrino
Copy link
Contributor Author

In reply to: yarnpkg/yarn#2438 (comment)

Fine with me. It is the core team who gonna work with it, so they must have their reasons. Most important is that autoclean is uncommon enough to be safe.

@bestander bestander merged commit 572187b into yarnpkg:master May 17, 2017
@bestander
Copy link
Member

Thanks, now who wants to send a PR?

@fliptheweb
Copy link

fliptheweb commented May 17, 2017

I read all threads about naming, cuz actually have the same misunderstanding and broke my build today (.yml in svgo).
Sorry for late, but what's about yarn diet or yarn weight-loss?

@rally25rs
Copy link
Contributor

I was about to open a new RFC for clean but I'll just add what I was going to send to this RFC. I would propose incorporating some additional changes in addition to a command rename:


Summary

Improve yarn clean command and documentation to prevent unintentional use.

Motivation

There is a long history of issues and errors that arise from people running yarn clean:

I would separate these into 2 major issues:

  1. People running yarn clean with hopes of it doing something useful, but it ends up deleting a required file.

For these cases, it is confusing because there is no list of what files will be deleted, and no confirmation when running the command. After it was run and deleted files, then you can view and modify .yarnclean but it is really too late at that point, as your files have been deleted already.

  1. People who accidentally ran this command. Perhaps someone meant to run their custom clean script with yarn run clean but accidentally ran yarn clean instead.

For these cases, there is no "undo" possible other than deleting node_modules, deleting .yarnclean and rerunning yarn install.

Detailed design

Several things we could do to improve the clean command:

  • Improve the documentation. Actually list the default files that will be deleted,a nd what the clean command really does.
  • Add a new command yarn clean init that does not delete any files, and only creates the .yarnclean file for review and editing. This would give users a chance to modify what is deleted before it happens.
  • Require a --force flag to actually run yarn clean. Attempting to run yarn clean would do nothing but output a warning that this is a potentially destructive operation, and that --force must be specified, and include a url to the documentation.
  • Running yarn clean --force would do what yarn clean does currently; actually delete the files (and create .yarnclean if it did not exist).

@ghost
Copy link

ghost commented Aug 10, 2017

I'm with @rally25rs

I'ld choose one of these:

  • document it and make it available for all to use as intended
  • or remove it completely if it's not for usage

if not possible I'ld be fine with a force flag or something but then still... document it

We migrated from npm to yarn and suddenly yarn clean did something totally unexpected and undocumented and I'm rather unsure if I should just use it and just stuff the .yarnclean file with everything I need to clean every once in a while or if this'ld be really really bad because it get's used internally for... something...

@BYK
Copy link
Member

BYK commented Aug 10, 2017

@rally25rs I think your addendum is great! I especially like the --force improvement. I don't think we need yarn clean init in that case. When you run yarn clean it would just run in dry mode, generate the .yarnclean file and tell this to the user. What do you say?

If so, a PR to make this happen before 1.0 would be greatly appreciated since this would be a breaking change.

@BYK
Copy link
Member

BYK commented Aug 10, 2017

I also agree with clean being too common, hence renaming. autoclean is not bad but my heart is with prune. That said we already had a discussion and an RFC merged for autoclean so I can live with both. Also, I'd expect people to have more scripts named prune then autoclean based on the numbers I pulled from my behind :D

@rally25rs
Copy link
Contributor

rally25rs commented Aug 10, 2017

The reason for the yarn clean to not generate the .yarnclean file was so that if someone accidentally did yarn clean then forgot to manually delete the .yarnlock, it wouldn't cause a clean on the next install or add. With a rename to autoclean that accident is less likely to happen.

"force" seems like a weird term in that context. What if we did --run?

What do you think about:

  • yarn autoclean - creates .yarnlock if it doesn't exist. (Initializes the AutoClean feature)
  • yarn autoclean --run - creates .yarnlock if it doesn't exist. run the clean (Runs the AutoClean feature)

If we all like that, I should have time to work up a PR this week.

@rally25rs
Copy link
Contributor

The more I think about it, the more I like:

  • yarn autoclean - Runs a "clean" only if .yarnclean exists, otherwise it tells the user to run yarn autoclean --init to opt-in to the autoclean process
  • yarn autoclean --init - Creates the default .yarnclean file if it does not exist, then tells the user to modify that file and yarn autoclean once they have modified it to fit their needs.

@BYK
Copy link
Member

BYK commented Aug 15, 2017

@rally25rs for some reason I still like using yarn autoclean -f more than the two-step init. Why/how did you get swayed away from the --force option to your new proposal? :)

@rally25rs
Copy link
Contributor

@BYK I could go either way... are you thinking:

yarn autoclean - only output a message to the user saying to add --force/-f to execute, and a link to the docs.

yarn autoclean -f - if .yarnclean exists, run the clean. If it does not exist, create it and notify the user to review/edit that file and run the command again.

@BYK
Copy link
Member

BYK commented Aug 24, 2017

@rally25rs I think we can create .yarnclean with yarn autoclean init and only do cleaning with yarn autoclean -f when .yarnclean exists. Makes sense?

Also do you think we can get this read for 1.0? That means we have about 1 week or so.

@rally25rs
Copy link
Contributor

@BYK ask and ye shall receive yarnpkg/yarn#4252

I didn't have time tonight to put together a PR for the website/docs. I'll get that tomorrow.

sbibauw added a commit to sbibauw/zsh-completions that referenced this pull request May 2, 2018
- `clean` has been renamed to `autoclean` (yarnpkg/rfcs#52)
- added description (from `yarn help [command]`) to `cache`, `import` and `versions`
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.

9 participants