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.

Sign up for GitHub

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

Jump to bottom

Only call update if no other source prop other than data differs #5745

Merged
merged 1 commit into from
Nov 27, 2017
Merged

Only call update if no other source prop other than data differs #5745

merged 1 commit into from
Nov 27, 2017

Conversation

bartvde
Copy link
Contributor

@bartvde bartvde commented Nov 23, 2017

This PR fixes #5731

@bartvde bartvde force-pushed the cluster-diff branch 2 times, most recently from b97313f to 7aedd2e Compare November 23, 2017 13:31
mourner
mourner requested changes Nov 24, 2017
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

If the source is clustered both before and after, but the cluster settings are different (e.g. clusterRadius), this case will break as well, right? So should we compare all properties of a source (except data) for a full fix?

@bartvde
Copy link
Contributor Author

bartvde commented Nov 24, 2017

Good point, will adapt

@bartvde
Copy link
Contributor Author

bartvde commented Nov 24, 2017

@mourner I added a6f57da but I'm unsure if it's okay to use union from lodash

mourner
mourner requested changes Nov 24, 2017
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

No, let's avoid the lodash dependency (the one already there will be removed shortly too #5744).

Since all those properties are not nested and with simple values, a short vanilla JS function with a for in loop will be enough.

@bartvde
Copy link
Contributor Author

bartvde commented Nov 24, 2017

thanks for the guidance @mourner I updated the PR

@bartvde bartvde changed the title Only call update if cluster setting is the same Only call update if no other prop other than data differs Nov 24, 2017
@bartvde bartvde changed the title Only call update if no other prop other than data differs Only call update if no other source prop other than data differs Nov 24, 2017
mourner
mourner reviewed Nov 24, 2017
View reviewed changes
src/style-spec/diff.js Outdated
commands.push({ command: operations.setGeoJSONSourceData, args: [sourceId, after[sourceId].data] });
} else {
updateSource(sourceId, after, commands, sourcesRemoved);
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can move the canUpdateGeoJSON condition to the parent if, which will remove the need for the else above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes made

@anandthakker
Copy link
Contributor

Thanks @bartvde !

@anandthakker anandthakker merged commit d94923c into mapbox:master Nov 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

setStyle diffing does not work if source gets clustered on the fly
3 participants
@bartvde @anandthakker @mourner