Skip to content

fix: Use native inflate if available - #1932

Open
elegaanz wants to merge 2 commits into
isomorphic-git:mainfrom
elegaanz:patch-1
Open

elegaanz wants to merge 2 commits into
isomorphic-git:mainfrom
elegaanz:patch-1

Conversation

@elegaanz

Copy link
Copy Markdown

Because supportsDecompressionStream was set to false by default, the test to see if native inflate was available or not was actually never run.

  • if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "fix: [Description of fix]"

@elegaanz

Copy link
Copy Markdown
Author

I don't know if I should squash my two commits together or not, please tell me what you prefer.

@elegaanz
elegaanz marked this pull request as ready for review June 13, 2024 14:03
@jcubic

jcubic commented Jun 13, 2024

Copy link
Copy Markdown
Member

thanks for the PR, it need to wait a bit until we fix the build pipeline.

@jcubic jcubic added the ready label Jun 13, 2024
@jcubic

jcubic commented Jun 13, 2024

Copy link
Copy Markdown
Member

No, you don't need to squash. I will do that when merging. I should probably remove that point from PR template.

@jcubic

jcubic commented Jun 22, 2024

Copy link
Copy Markdown
Member

I disabled broken GitLab test, can you rebase?

elegaanz and others added 2 commits June 24, 2024 10:37
Because `supportsDecompressionStream` was set to `false` by default, the test to see if native inflate was available or not was actually never run.
@mojavelinux

Copy link
Copy Markdown
Contributor

I agree that this seems to have been a coding error. The code was added but never used.

In order for this to actually work when running tests in Node.js, the following needs to be added to jest.config.cjs as well:

  globals: {
    Blob: global.Blob,
    CompressionStream: global.CompressionStream,
    DecompressionStream: global.DecompressionStream,
    Response: global.Response
  },

This will be related to the fix for #1933.

@mojavelinux

Copy link
Copy Markdown
Contributor

Hold on. I just did some quick performance testing and determined that using the DecompressionStream in Node.js performs terribly when compared to pako (and even worse when compared to the built-in zlib). This would be a very bad change to make now if it truly does have that consequence. On one large repository, switching to DecompressionStream caused the kernel to kill the Node.js process. It's possible the there is a leak in the isomorphic-git code as well.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants