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

Remove *.gz from default .yarnclean #4450

Closed
nkbt opened this issue Sep 14, 2017 · 10 comments
Closed

Remove *.gz from default .yarnclean #4450

nkbt opened this issue Sep 14, 2017 · 10 comments

Comments

@nkbt
Copy link

nkbt commented Sep 14, 2017

Do you want to request a feature or report a bug?
bug

What is the current behavior?
autoclean command has *.gz in default .yarnclean file.
https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/autoclean.js#L56

Problem: some packages are coming with gz to reduce package size

I reckon it is quite a minor issue, but not obvious why required files are missing.

If the current behavior is a bug, please provide the steps to reproduce.

docker run -it mhart/alpine-node:8
# then
rm -rf /test
mkdir /test
cd /test
cat > package.json <<- EOM
{
  "name": "test",
  "version": "0.0.1",
  "dependencies": {
    "left-pad": "*"
  }
}
EOM
yarn install
yarn autoclean --init
yarn remove left-pad
yarn add @std/esm
find .

Result:

/test # find .
.
./package.json
./.yarnclean
./node_modules
./node_modules/.yarn-integrity
./node_modules/@std
./node_modules/@std/esm
./node_modules/@std/esm/package.json
./node_modules/@std/esm/LICENSE
./node_modules/@std/esm/index.js
./yarn.lock
/test #

Running same code but omitting yarn autoclean --init line:

/test # find .
.
./package.json
./node_modules
./node_modules/.yarn-integrity
./node_modules/@std
./node_modules/@std/esm
./node_modules/@std/esm/README.md
./node_modules/@std/esm/package.json
./node_modules/@std/esm/LICENSE
./node_modules/@std/esm/index.js
./node_modules/@std/esm/esm.js.gz
./yarn.lock

What is the expected behavior?
gz should stay

Please mention your node.js, yarn and operating system version.

#OS: mhart/alpine-node:8

/ # node -v
v8.5.0
/ # yarn -v
1.0.1
/ #
@BYK
Copy link
Member

BYK commented Sep 14, 2017

Sounds fair. Would you like to submit a PR?

@BYK
Copy link
Member

BYK commented Sep 14, 2017

That said, this would be a breaking change for people relying on this behavior.

@nkbt
Copy link
Author

nkbt commented Sep 15, 2017

Sure, will do a PR.

This is only a default value for cleanup. Just removing *.gz might be a one solution.

Alternatively (or in addition) - maybe worth giving some note to user if some files were cleaned up. Like basic stats. For example:

X files matched by .yarnclean and were automatically removed, Y Mb space saved

In this case it will be both helpful and will act as an indication/reminder that there is an auto-cleaning process in place.

@BYK
Copy link
Member

BYK commented Sep 15, 2017

The thing with autoclean is, it doesn't run automatically unless you set it up with a .yarnclean file. And when you do, you can override this list in that file so not sure showing a list of files is that useful. What do you think?

@nkbt
Copy link
Author

nkbt commented Sep 16, 2017

My problem was, that I personally set up autoclean couple of months ago and completely forgot about it, because it is 'auto'. Showing list of files would be too heavy, I was more thinking about one line stats/reminder that there are some files deleted by autoclean. This way it would be impossible to forget about it.

Regarding pr it may be not as simple as removing one line. I ran tests yesterday with gz removed and got like 30 tests failing. Reverted to master and still had about 7 test failing. This is strange since I could see tests pass on circleci. I am on the latest macbook pro and tests run for over 2 minutes. Will have to dig a bit more to figure out why tests may fail on master branch first.

@jdalton
Copy link
Contributor

jdalton commented Sep 23, 2017

Just a heads up, Lodash v5 will ship with gz modules.

@BYK
Copy link
Member

BYK commented Sep 26, 2017

I think it is reasonable to remove .gz from the default list then. I also think @nkbt's idea of informing the user about what is being deleted or what was deleted after the fact is a good idea.

That means 2 PRs. Anyone up for it? They should be fairly simple.

@nicobarray
Copy link
Contributor

Not sure if @nkbt is working on the one line reminder. If not, I can give it a try. I will start by removing the .gz from the default list too :)

@nicobarray
Copy link
Contributor

I just made a PR to remove the *.gz line from the default filters used to generate the default .yarnclean file (PR #4601)

I'll prepare something for the one-line reminder in the meantime.

@nicobarray
Copy link
Contributor

nicobarray commented Oct 1, 2017

It looks like that number of file remove + the saved space is already reported as info after a yarn autoclean --force. (#659 for the freed space size)

I think it matches what @nkbt wanted.

BYK pushed a commit that referenced this issue Oct 2, 2017
**Summary**

I removed the *.gz from the default .yarnclean generated by the `yarn autoclean --init` command. 

**Test plan**

I tested by hand and launch `yarn test` and didn't find a test case for the autoclean --init option. If needed I could write a test case to check the default .yarnclean file content against the constant DEFAULT_FILTER.

I hope it resolve half the issue #4450 :)
@BYK BYK closed this as completed Oct 3, 2017
joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
…#4601)

**Summary**

I removed the *.gz from the default .yarnclean generated by the `yarn autoclean --init` command. 

**Test plan**

I tested by hand and launch `yarn test` and didn't find a test case for the autoclean --init option. If needed I could write a test case to check the default .yarnclean file content against the constant DEFAULT_FILTER.

I hope it resolve half the issue yarnpkg#4450 :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants