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

Warn when trying to register two interactions with the same ID #1151

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jun 30, 2022

Now, when this occurs, the second interaction no longer replaces the first. See #1143 (comment)

@axelboc
Copy link
Contributor Author

axelboc commented Jun 30, 2022

Should we throw instead? 🤔

One one hand, this is a developer error, so it would make sense to throw so the problem gets caught during development. Not every app has feature tests that crash when warnings are logged...

On the other hand, I can imagine situations where multiple interaction components with the same ID are rendered at the same time in production due to an unlikely combination of states that was missed during development... Crashing the entire vis because of that would not be fun for users.

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Warning instead of throwing is fine.

If this happens during dev, it is likely that it is because we are testing and/or adding a new interaction so we are likely to investigate if the interaction is not properly added.

@axelboc axelboc force-pushed the group-interaction-stories branch from 47efcdf to 62d78b7 Compare July 1, 2022 09:09
Base automatically changed from group-interaction-stories to main July 1, 2022 09:14
@axelboc axelboc force-pushed the warn-duplicate-interaction branch from d3190a8 to 669bc64 Compare July 1, 2022 09:16
@axelboc axelboc merged commit c862e89 into main Jul 1, 2022
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