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

fix: enable DecompressionStream detection #1799

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DenSpirit
Copy link

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
Copy link
Contributor

jcubic commented Aug 2, 2023

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
Copy link
Author

DenSpirit commented Oct 30, 2023

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
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
Copy link
Contributor

jcubic commented Feb 11, 2024

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
Copy link
Contributor

jcubic commented Feb 15, 2024

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
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
Copy link
Contributor

jcubic commented Feb 15, 2024

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.

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