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

Additional error checking in Compression API #732

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Jun 3, 2023

Perform more error checking for DecompressionStream, which requires adding a new compat flag.

@fhanau fhanau requested review from dom96 and jasnell June 3, 2023 18:05
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment, but otherwise changes look good so accepting to unblock.

Comment on lines 401 to 402
auto& ioContext = IoContext::current();
auto flags = ioContext.getWorker().getIsolate().getApiIsolate().getFeatureFlags();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat suspicious of this, mainly because I haven't seen feature flags be accessed like this before. I suppose if it works it's fine, but iirc jsg will get you the feature flags if you just put them in the method (CompatibilityFlags::Reader featureFlags) and I see a lot of other code doing it this way so might be better to be consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, switched to getting the compat flags from JSG.
I found the old approach in a global scope method (ServiceWorkerGlobalScope::request) and was initially going to use it in pumpOnce() directly, but since the flags check is now in the constructor it is certainly cleaner to have them in the function parameters.

@fhanau fhanau force-pushed the felix/web-compression-errors branch 2 times, most recently from 1986d91 to c8d5e24 Compare June 5, 2023 14:18
Perform more error checking for DecompressionStream,
which requires adding a new compat flag
@fhanau fhanau force-pushed the felix/web-compression-errors branch from c8d5e24 to 4460d51 Compare June 5, 2023 17:22
@fhanau fhanau merged commit 8542629 into main Jun 5, 2023
@fhanau fhanau deleted the felix/web-compression-errors branch June 5, 2023 18:12
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.

3 participants