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

feat: make the variable map return early for noops #6674

Merged
merged 4 commits into from
Dec 2, 2022

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Dec 1, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6524

Proposed Changes

  1. Makes it so that the variable map returns early in cases where renames are noops (other modificiations already did this correctly).
  2. Moves firing create events from the VariableModel to the VariableMap.

Reason for Changes

  1. This prevents us from getting into infinite event loops when sharing events between multiple workspaces. See here for an example/explanation.
  2. This allows us to use variable models as backing data models without actually having them (e.g.) show up in the flyout.

Test Coverage

Added unit tests for creating, deleting, and renaming, and when they should fire events vs not fire events.

Documentation

N/A

Additional Information

This is not dependent on any parent branches, so it's ready for review!

@BeksOmega BeksOmega requested a review from a team as a code owner December 1, 2022 21:09
@github-actions github-actions bot added the PR: feature Adds a feature label Dec 1, 2022
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Dec 2, 2022
@BeksOmega BeksOmega merged commit fc2c10d into google:develop Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor the VariableMap to return early
2 participants