Skip to content

fix: enable DecompressionStream detection - #1799

Open
DenSpirit wants to merge 3 commits into
isomorphic-git:mainfrom
DenSpirit:enable-test-decompression-stream
Open

DenSpirit wants to merge 3 commits into
isomorphic-git:mainfrom
DenSpirit:enable-test-decompression-stream

Conversation

@DenSpirit

Copy link
Copy Markdown

I'm fixing a bug or typo

  • 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]"

@jcubic

jcubic commented Aug 2, 2023

Copy link
Copy Markdown
Member

Can you explain what this fixes? It's not clear what is the purpose if this change.

Can you write a unit test that fails before the fix and works after the fix?

@DenSpirit

DenSpirit commented Oct 30, 2023 •

Copy link
Copy Markdown
Author

Hi. Sorry for taking so long to reply.
DecompressionStream support is supposed to be detected by isogit the first time it's requested. isogit checks for that by examining global *variables.
A value of false in supportsDecompressionStream implies that DecompressionStream detection was run and the result was negative, while null would imply that the detection was not run yet.

Because it's set to false in code, the detection never runs and isogit treats DecompressionStream as non-existing (and defaults to pako) at all times.

If you look at a neighbouring file deflate.js, you'll see it has a line let supportsCompressionStream = null, which makes CompressionStream available for discovery. This merge request does the same for DecompressionStream.

At some point I'll provide a unit test, but I can't say when exactly yet, sorry.

@DenSpirit
DenSpirit force-pushed the enable-test-decompression-stream branch 2 times, most recently from 269bd0d to 7076e68 Compare February 11, 2024 19:48
@DenSpirit

Copy link
Copy Markdown
Author

I have added the unit tests, unfortunately they can't follow the regular style due to usage of local variables.
I am getting errors on certain browser versions, does it mean that the DecompressionStream support check is incomplete and more checks have to be done? I have already added a condition that was added to CompressionStream but not to DecompressionStream, probably that is not enough.

If possible, please assist :)

@jcubic

jcubic commented Feb 11, 2024

Copy link
Copy Markdown
Member

There are two issues:

  1. recent changes in The test stream needs to be closed #1864 the stream needs to be closed
  2. The tests are failing because of Android (probably some recent change with Chrome browser) I don't know what to do yet

@jcubic

jcubic commented Feb 15, 2024

Copy link
Copy Markdown
Member

The test got fixed, Please rebase and close the stream as on CompressionStream.

@DenSpirit
DenSpirit force-pushed the enable-test-decompression-stream branch from 7076e68 to 932eaa3 Compare February 15, 2024 12:25
@DenSpirit
DenSpirit force-pushed the enable-test-decompression-stream branch from 932eaa3 to 4ff776c Compare February 15, 2024 14:06
@DenSpirit

Copy link
Copy Markdown
Author

Made the necessary changes but it appears that the tests are still failing. I'm not sure the reason is my changes still, the failures seem unrelated.

@jcubic

jcubic commented Feb 15, 2024

Copy link
Copy Markdown
Member

It may be related to changes in Zlib library now that decompressionStream is actually used it produces differnet output. I'm not sure how since decompression should unpack old and new data in same way.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants