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: import Observable as a type #826

Merged
merged 2 commits into from
May 7, 2023
Merged

Conversation

spheenik
Copy link
Contributor

@spheenik spheenik commented May 6, 2023

I think I found it.
Possibly fixes issue #825

@spheenik spheenik changed the title import Observable as a type fix: import Observable as a type May 6, 2023
@stephenh
Copy link
Owner

stephenh commented May 6, 2023

Ah yep, this seems reasonable; thanks for the PR! Let's see what the test suite says.

Fwiw to get the PR green, you'll need to update the integration/ test output, and push it up as part of this PR, since that serves as our integration tests; there should be instructions in the readme, but let me know if you have questions. Thanks!

@spheenik
Copy link
Contributor Author

spheenik commented May 6, 2023

Hi Stephen,
did as described in the readme.

It turned out that you use observableType() in types.ts both for creating the "generated api" as well as your test cases.
So I opted to introduce a new parameter asType to it, which is false by default, and only set it to true when the api generation happens.

Hope I did everything right.

@stephenh
Copy link
Owner

stephenh commented May 7, 2023

@spheenik fwiw I'm not 100% following; it's been awhile since I've looked at the Observable test suites, but can you link to where adding the t: broke a test?

Also fwiw it looks like, at least in the current PR, the asType is never passed true, so I don't believe there is any behavior change at the moment.

@spheenik
Copy link
Contributor Author

spheenik commented May 7, 2023

Lol, I set it to false... I just corrected it.

@spheenik
Copy link
Contributor Author

spheenik commented May 7, 2023

It breaks a single test when it is imported by type:

Summary of all failing tests
 FAIL  integration/grpc-web/example-test.ts
  ● Test suite failed to run

    integration/grpc-web/example.ts:983:16 - error TS1361: 'Observable' cannot be used as a value because it was imported using 'import type'.

    983     return new Observable((observer) => {
                       ~~~~~~~~~~

      integration/grpc-web/example.ts:5:15
        5 import type { Observable } from "rxjs";
                        ~~~~~~~~~~
        'Observable' was imported here.

@stephenh
Copy link
Owner

stephenh commented May 7, 2023

Ah great! Thanks for including the snippet that failed; that makes sense, and verifies we've got existing regression coverage for this change, so don't need any new tests for this PR. Thanks for the fix!

@stephenh stephenh merged commit 52e84ba into stephenh:main May 7, 2023
6 checks passed
stephenh pushed a commit that referenced this pull request May 7, 2023
## [1.147.2](v1.147.1...v1.147.2) (2023-05-07)

### Bug Fixes

* import Observable as a type ([#826](#826)) ([52e84ba](52e84ba))
@stephenh
Copy link
Owner

stephenh commented May 7, 2023

🎉 This PR is included in version 1.147.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

2 participants