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

[preview] Don't write ref attributes on materialized values #580

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Feb 6, 2018

When the preview was called with a value that was materialized, it was extending it afterwards with the original passed value. This caused the _ref (and sometimes _key) attributes to be added to the resulting value, and could potentially cause subsequent calls with the same value to identify it as a reference and (needlessly) materialize it again.

This patch omits writing the original attributes if the passed value was a reference.

@bjoerge bjoerge requested a review from rexxars February 6, 2018 13:39
Copy link
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

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

Looks good to me

@@ -36,12 +36,16 @@ export default function createPathMaterializer(observeWithPaths) {

const nextHeads = uniq(missingHeads.map(path => [path[0]]))

const _isRef = isReference(value)
Copy link
Member

Choose a reason for hiding this comment

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

Why the underscore? I thought this was pretty much only used to mark private methods/properties in classes and similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just me being a scatterbrain. Originally called the variable _isReference due to conflict with the function name, then decided to shorten it. Fixed :)

@bjoerge bjoerge force-pushed the preview-dont-set-ref-on-materialized-value branch from f663036 to 3c0514a Compare February 7, 2018 09:24
@bjoerge bjoerge merged commit f95dfaa into next Feb 7, 2018
@bjoerge bjoerge deleted the preview-dont-set-ref-on-materialized-value branch February 7, 2018 09:24
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