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

ViewFactory: add variant of makeMessageListContainerModifier with channel #430

Conversation

AndrewSB
Copy link
Contributor

@AndrewSB AndrewSB commented Feb 7, 2024

i've just added a new view factory function with a channel param. shouldn't be a breaking change

☑️ Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@AndrewSB AndrewSB requested a review from a team as a code owner February 7, 2024 19:14
@martinmitrevski
Copy link
Contributor

Hey @AndrewSB, thanks for raising the PR. It's not a breaking change, but it will be confusing for our customers to have 2 versions of the same method and long term it won't be scalable for us to maintain that. Therefore, we will close this PR, but will keep it in mind for a major version release.

@AndrewSB
Copy link
Contributor Author

AndrewSB commented Feb 8, 2024

got it, i'm going to have to maintain a fork until this is included upstream, so i'd really appreciate this making it into your version

@martinmitrevski
Copy link
Contributor

one idea to overcome this: keep the currently selected channel as a var in your factory and then pass it in the modifier method we already have.

@AndrewSB
Copy link
Contributor Author

AndrewSB commented Feb 9, 2024

could do, but i'm worried about some sort of edge case where we don't clear that channel/we deeplink into another channel and don't update the var in time

@AndrewSB
Copy link
Contributor Author

@martinmitrevski what if we added an unsafe_ prefix to this function?

it would technically enter your API compatibility, but with the unsafe prefix, i think it would be totally fair for you to break the API once there's a major release?

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.

2 participants