search

grpc / grpc-dart

The Dart language implementation of gRPC.
https://pub.dev/packages/grpc
Apache License 2.0
835 stars 256 forks source link

Fix issue 669 #693

Closed RubenGarcia closed 2 months ago

RubenGarcia commented 3 months ago

https://grpc.github.io/grpc/core/md_doc_compression.html indicates that compression flag should not be set if grpc-encoding is identity.

This PR makes grpc-dart set the compression flag properly in this case.

mosuem commented 3 months ago

Thanks a lot for the investigation and the fix! If you could add a changelog entry describing your change (with a link to this PR), as well as adding the information from https://github.com/grpc/grpc-dart/issues/669#issuecomment-2006654280 to this PR description, that would help a lot.

RubenGarcia commented 3 months ago

Thanks a lot for the investigation and the fix! If you could add a changelog entry describing your change (with a link to this PR), as well as adding the information from #669 (comment) to this PR description, that would help a lot.

Done and fixed the two issues with the workflow.

RubenGarcia commented 3 months ago

I´m getting this error in the other workflow:

Validating package:grpc 3.2.4 (dir=/home/runner/work/grpc-dart/grpc-dart):grpc
pubspec:
  - version: 3.2.4
changelog:
* Set compressed flag correctly for grpc-encoding = identity. Fixes [#669](https://github.com/grpc/grpc-dart/issues/669) (https://github.com/grpc/grpc-dart/pull/693)
error: pubspec version (3.2.4) and changelog (null) don't agree

Since 3.2.4 has already released, I will change both to 3.2.5.

RubenGarcia commented 3 months ago

Looking good! If you could add a small test for this to not risk having regressions, we can merge.

Test added.

RubenGarcia commented 2 months ago

The error when running dart / test (windows-latest) when running dart run --enable-vm-service --timeline-streams=Dart test/timeline_test.dart Could not find a command named "/hostedtoolcache/windows/dart/3.4.0-260.0.d... seem to be unrelated to my changes.

mosuem commented 2 months ago

Right, I will take a look.

mosuem commented 2 months ago

https://github.com/grpc/grpc-dart/issues/697

RubenGarcia commented 2 months ago

Is there anything else needed here?

github-actions[bot] commented 2 months ago

PR Health

Breaking changes :heavy_check_mark:

Details | Package | Change | Current Version | New Version | Needed Version | Looking good? | | :--- | :--- | ---: | ---: | ---: | ---: | |grpc|None|3.2.4|3.2.5|3.2.4|:heavy_check_mark:|

Changelog Entry :heavy_check_mark:

Details | Package | Changed Files | | :--- | :--- | Changes to files need to be [accounted for](https://github.com/dart-lang/ecosystem/wiki/Changelog) in their respective changelogs.

Coverage :heavy_check_mark:

Details | File | Coverage | | :--- | :--- | |lib/src/shared/message.dart| :green_heart: 76 % | This check for [test coverage](https://github.com/dart-lang/ecosystem/wiki/Test-Coverage) is informational (issues shown here will not fail the PR).

API leaks :warning:

Details The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API. | Package | Leaked API symbols | | :--- | :--- | |grpc|$1.Duration
ServerHandler
Any| This check can be disabled by tagging the PR with `skip-leaking-check`.

Package publish validation :heavy_check_mark:

Details | Package | Version | Status | | :--- | ---: | :--- | | package:grpc | 3.2.5 | **ready to publish** | Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.
RubenGarcia commented 2 months ago

The failed tests complaining about dart:js_interop_unsafe seem to be independent of my changes, although they are a transitive dependency of one of my imports.