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

HOTFIX: fix subscription narrowing problem #306

Closed
wants to merge 2 commits into from

Conversation

vejrj
Copy link
Contributor

@vejrj vejrj commented May 11, 2023

No description provided.

@vejrj vejrj requested review from freiksenet and alloy and removed request for freiksenet May 11, 2023 08:52
Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

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

I’m missing an example of what the resolver implementation would look like. Is this requiring folks to include the kind property in their implementation?

@vejrj vejrj force-pushed the jvejr/subscription-fix-narrowing-problem branch from 78dac2f to 5842456 Compare May 11, 2023 09:27
@vejrj
Copy link
Contributor Author

vejrj commented May 11, 2023

I’m missing an example of what the resolver implementation would look like. Is this requiring folks to include the kind property in their implementation?

Yes, you will need to add kind for "SubscribeAndResolve"

@alloy
Copy link
Member

alloy commented May 11, 2023

That doesn’t seem helpful. At that point it’s more intuitive to export two different type definitions.

@vejrj
Copy link
Contributor Author

vejrj commented May 11, 2023

That doesn’t seem helpful. At that point it’s more intuitive to export two different type definitions.

That was my initial idea, but @freiksenet said that we would need to refactor everything then.

@alloy
Copy link
Member

alloy commented May 11, 2023

Can you elaborate on the difference in effort needed to add kind vs using a different type? Both can be done relatively the same way with a codemod afaik.

Copy link
Member

@alloy alloy left a comment

Choose a reason for hiding this comment

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

As discussed offline, I consider this solution a no-go. We provide framework solutions and we need to consider our customers [the product engineers], and this workaround is not intuitive. We should:

  1. Figure out if it's simply not possible to provide a single type and have TypeScript correctly narrow the union;
  2. and otherwise provide 2 types and have the product engineer choose the right one—which is something that they can intuitively understand.

@freiksenet freiksenet closed this May 15, 2024
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