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(*): delete archieve after uncompressing it #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stepancar
Copy link

@stepancar stepancar commented Feb 23, 2024

du -sh ./node_modules/nw/*
8.0K    ./node_modules/nw/README.md
8.0K    ./node_modules/nw/bin
20K     ./node_modules/nw/lib
624K    ./node_modules/nw/node_modules
470M    ./node_modules/nw/nwjs
149M    ./node_modules/nw/nwjs-v0.82.0-linux-x64.tar.gz    <----- ???
4.0K    ./node_modules/nw/package.json

Ideally we should not store archive after uncompressing it.
This is important for systems which run nwjs in docker environment.
This change reduces package size by 20%

@ayushmanchhabra
Copy link
Collaborator

The tarball is not deleted so that the ./node_modules/nw directory can be used as nw-builder's cacheDir. I plan on porting nw-builder's get mode to this package which should then allow you to enable/disable cache (keep/remove tarball).

@stepancar
Copy link
Author

@ayushmanchhabra , what do you think if we add an environment variable NWJS_KEEP_TARBALL (default value is false) for npm-installer
which would be used by nw-builder?

@ayushmanchhabra
Copy link
Collaborator

That works! I suggest NWJS_CACHE with a default value of false since linux builds are compressed as tarballs while Mac and Windows as zips.

@stepancar
Copy link
Author

stepancar commented Mar 1, 2024

@ayushmanchhabra , what do you think if we do some refactoring here?
I don't think NWJS_CACHE makes sense.

nwCache in npm-installer has description Path to the compressed file.
Lets rename it to pathToCompressedFile?

we could call env variable NWJS_KEEP_COMPRESSED_FILE then.

But lets firstly understand cases we try to support

If I understand correctly

  1. scenario
    we call NWJS_URLBASE=file:path/to/compressed/files/somewhere/locally npm install we don't need to delete compressed files after uncompressing
  2. scenario
    we call NWJS_URLBASE=https://path/to/compressed/files/somewhere/in/the/internet npm install
    we don't need compressed files after uncompressing
  3. scenario
    we call npm install // download compressed file from default binary mirror
    we don't need compressed files after uncompressing

Could you please explain how #131 (comment) works exactly?
Which command are you running?

@stepancar
Copy link
Author

@ayushmanchhabra may I ask you to look at my comment? Thank you!

@ayushmanchhabra
Copy link
Collaborator

ayushmanchhabra commented Mar 11, 2024

Apologies for the late response! Forgot to add myself as reviewer and hence PR got lost in the notifications.

nwCache in npm-installer has description Path to the compressed file.
Lets rename it to pathToCompressedFile?

Sounds good!

Could you please explain how #131 (comment) works exactly?

nw-builder caches multiple NW.js binaries:

/path/to/cache/nwjs-sdk-v0.85.0-linux-x64.tar.gz
/path/to/cache/nwjs-sdk-v0.85.0-win-ia32.zip

To use ./node_modules/nw/nwjs as its cache dir, it has to preserve the tarballs and zips due to this edge case.

await nwbuild({
  version: '0.85.0',
  cacheDir: './node_modules/nw',
  srcDir: './src',
  outDir: './out/desktop',
  glob: false
});

The plan is to move nw-builder's get and run mode to nwjs/npm-installer so that nw-builder can focus on building and packaging. I should be able to port this over by Wednesday. Does that work for you?

@stepancar
Copy link
Author

stepancar commented Mar 12, 2024

Apologies for the late response! Forgot to add myself as reviewer and hence PR got lost in the notifications.

nwCache in npm-installer has description Path to the compressed file.
Lets rename it to pathToCompressedFile?

Sounds good!

Could you please explain how #131 (comment) works exactly?

nw-builder caches multiple NW.js binaries:

/path/to/cache/nwjs-sdk-v0.85.0-linux-x64.tar.gz
/path/to/cache/nwjs-sdk-v0.85.0-win-ia32.zip

To use ./node_modules/nw/nwjs as its cache dir, it has to preserve the tarballs and zips due to this edge case.

await nwbuild({
  version: '0.85.0',
  cacheDir: './node_modules/nw',
  srcDir: './src',
  outDir: './out/desktop',
  glob: false
});

The plan is to move nw-builder's get and run mode to nwjs/npm-installer so that nw-builder can focus on building and packaging. I should be able to port this over by Wednesday. Does that work for you?

Thank you for the response.
I'm not sure I'm following about nw-builder. I will wait for you PR to nw-builder for better understanding. Thank you!

@stepancar
Copy link
Author

I think now I understand.
You are going to move https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32
into nw-installer. so it will be not just a standard npm package which downloads binaries and puts it into node_modules, but it should also provide an alternative to https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32

@ayushmanchhabra
Copy link
Collaborator

I think now I understand. You are going to move https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32 into nw-installer. so it will be not just a standard npm package which downloads binaries and puts it into node_modules, but it should also provide an alternative to https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32

Exactly.

@stepancar
Copy link
Author

I think now I understand. You are going to move https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32 into nw-installer. so it will be not just a standard npm package which downloads binaries and puts it into node_modules, but it should also provide an alternative to https://github.com/nwutils/nw-builder/blob/0ca3c341f312b584b37628a9d3726eca2b4584be/src/get/index.js#L32

Exactly.

Ok, Thank you!
So, I will wait for your PR then and after that I will check what I can improve in npm installer.

@ayushmanchhabra
Copy link
Collaborator

@stepancar ping

@stepancar
Copy link
Author

@ayushmanchhabra , hello, I think I will work on it on friday, thanks for the ping

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.

None yet

2 participants