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

Removing memoisation from context #1312

Merged
merged 2 commits into from Oct 29, 2021
Merged

Conversation

mattgperry
Copy link
Collaborator

@mattgperry mattgperry commented Oct 28, 2021

Removing context memoisation ensures values removed from

Fixes https://github.com/framer/company/issues/23061

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 28, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e48a15c:

Sandbox Source
Framer Motion: Simple animation Configuration
App Store UI using React and Framer Motion Configuration
Framer Motion: Reorder animation Configuration
Framer Motion: growing item positionTransition issue Configuration
Framer Motion: Image lightbox Configuration


describe("animate prop as variant", () => {
// // @ts-ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, oops. What caused this?

@mattgperry mattgperry requested a review from nvh October 28, 2021 14:46
variantLabelsAsDependency(animate),
]
: []
[variantLabelsAsDependency(initial), variantLabelsAsDependency(animate)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the useMemo needed at all if we break it?

And shall we add a comment as to why we intentionally break the memoization in some cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't feel like it's necessary. We only generate a new context object when its contents change, it feels like a relatively straightforward use of memoisation.

/**
* In a future refactor we can replace the features-as-components and
* have this loop through them all firing "effect" listeners
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment no longer relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it was something I thought was a nice idea at the time but don't now.

@mattgperry mattgperry added the automerge Land this PR label Oct 29, 2021
@mergetron mergetron bot merged commit b040f98 into main Oct 29, 2021
@mergetron mergetron bot deleted the fix/remove-context-memoisation branch October 29, 2021 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Land this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants