Skip to content

Load GeoJSON data on addLayer in a store plugin #553

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

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

pakb
Copy link
Contributor

@pakb pakb commented Dec 1, 2023

so that this logic can be shared between Cesium and OL (was implemented twice in each "flavor")

the styleFromLiterals.js file was modifying values received as function args, I changed that because it was triggering store error (as sometimes, these arguments were coming straight from the store, and are thus immutable outside of a store action)

I also changed the store action "updateLayer" so that the payload can be directly a layer (no need to wrap it in an object anymore)

Test link

@pakb pakb requested a review from ltshb December 1, 2023 06:52
store.commit('updateLayer', { layer: updatedExternalLayer })
store.dispatch('updateLayer', updatedExternalLayer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my comprehension why changing from commit to dispatch here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that it may go through the layer validation present, post-commit, in the action.
But that isn't mandatory, just that I prefer not touching mutation much, and using action whenever I can (only store modules themselves should use mutations)
It will greatly ease the future migration to pinia not to use mutation in the code outside of store modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok good to know I'll try to stick to actions then

return new Promise((resolve, reject) => {
axios
.get(geoJsonLayer.styleUrl)
.then((response) => response.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if the service return something else as 200 ? I'm not sure that axios is raising an exception in this case. Is this function only calling our backend ? Because if not I would consider adding a timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only our backend is requested, with URLs coming from our backend listing of layers. I don't think it make much sense to double proof the content of what is served

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@pakb pakb force-pushed the feat_load_geojson_data_in_store branch from 32c0ad1 to ef4d180 Compare December 1, 2023 10:26
@pakb pakb requested a review from ltshb December 1, 2023 10:26
@pakb pakb force-pushed the feat_load_geojson_data_in_store branch from ef4d180 to a4b3d68 Compare December 1, 2023 10:49
so that this logic can be shared between Cesium and OL (was implemented twice in each "flavor")

the styleFromLiterals.js file was modifying values received as function args, I changed that because it was triggering store error (as sometimes, these arguments were coming straight from the store, and are thus immutable outside of a store action)

I also changed the store action "updateLayer" so that the payload can be directly a layer (no need to wrap it in an object anymore)
@pakb pakb force-pushed the feat_load_geojson_data_in_store branch from a4b3d68 to f115896 Compare December 1, 2023 10:54
@pakb pakb merged commit 53e6538 into develop Dec 4, 2023
@pakb pakb deleted the feat_load_geojson_data_in_store branch December 4, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants