Closed bencpeters closed 4 years ago
There may be a few things to consider:
Most types will be lost if we asynchronously load configs (admin_boundries.json
is unaffected). Types will have to be constructed elsewhere - relatively easy
Default JSON files should be accessible from the repo for tests or local development to run smoothly
To keep deployment flexible, there should be a method to specify a location for the JSON files.
.env
variable such as REACT_APP_CONFIG_SOURCE=example.com
where file names are kept constant and added on as a path: example.com/prism.json
.env
variable for every possible JSON fileI believe the best approach is to setup a new Redux slice to handle the async loading and storage of these config files.
We can then setup each affected component to check for availability and dispatch for a config load, similar to how loadLayerData is implemented.
Once we have a clear idea on how to address the caveats I mentioned above, I can look into tackling this issue within the MLH Fellowship Fork!
An important clarification: I'm not imagining that we'd load ALL the config files asynchronously - we want the static build to have access to the layer metadata & other files that set up the navbar (what layers are available, layer categories, etc.). What I'd like to load asynchronously are the large data geojson files that get loaded with GeoJSON layers.
* Most types will be lost if we asynchronously load configs (`admin_boundries.json` is unaffected). Types will have to be constructed elsewhere - relatively easy
For some files we already are "losing" type information because of the snake_case
-> camelCase
translation for keys (this doesn't apply to the admin boundaries, but it is true for the layer data). In general though, I'd rather be explicit about the types that we expect (and ideally check incoming JSON data to confirm that it conforms to those expectations) rather than rely on implicit types from the JSON files, so I don't think the requirement to manually define some types for this is a problem.
Default JSON files should be accessible from the repo for tests or local development to run smoothly
We could easily exclude files from the build for dev purposes by checking for a dev flag on the build environment, but I'd prefer to use the same process to load the files in dev & prod. So if we provide dev versions of the JSON files, they should still be served up asynchronously by the development server rather than import
ed directly.
I don't think we actually need a new Redux slice for this - these files are just other examples of layer data, so we can use the existing asynchronous thunk for loading layer data to load these files. There are already placeholder loading functions for NSO Layers and Groundstation layers that just need to be updated to do the loading.
I think we already have a fairly good mechanism for configuring the location of the layer data files - the layer definitions themselves are designed to be flexible and can specify a path
to the file. We may need to update the layer types slightly for the admin_boundaries
file, since that's used in a number of different layers (especially the impact layers), but I think we can accommodate all the configuration we need through the layer definitions.
Ah I see, thanks for the clarification. To make sure we are on the same page, only the following files will be moved to this new async loading system:
admin_boundries.json
The files above will be moved to public/data and will be loaded using fetch().
NSO type layers in layers.json
will have their path
variable be repurposed to point to a file instead of a variable name. This can be a file path or web location.
OR
We can adopt this pattern used by groundstation layers, allowing us to provide both a prod and dev location.
Let me know which you prefer.
I don't think we actually need a new Redux slice for this - these files are just other examples of layer data, so we can use the existing asynchronous thunk for loading layer data to load these files.
We probably need to do something with Redux for admin_boundries.json
right? It could fit within the mapSlice. Location also needs to be defined somewhere. I propose a default of data/admin_boundries.json
with an .env option to reconfigure such as REACT_APP_ADMIN_BOUNDRIES_LOCATION
... There are already placeholder loading functions for NSO Layers and Groundstation layers that just need to be updated to do the loading.
From looking at the code, it seems groundstation layers are already loaded correctly; I'll use it as a rough guide for NSO.
We may need to update the layer types slightly for the admin_boundaries file, since that's used in a number of different layers (especially the impact layers),
Not sure what you mean by this exactly - still trying to piece together what this file stores exactly besides the boundary polygons. If some edit is needed in this file, a deeper insight into what you mean would be great, then I'll add to our game-plan if needed :)
I think I'd prefer that we didn't have the config files have a bunch of "backup" definitions in them, let's just keep it to a single URL location for now. So each layer that depends on an external data file has a path for that file.
For now, we can even just keep the data files hosted in public
and deploy them with the app. This will mean that the config files won't have to change between dev and prod, but will still keep the GeoJSON files from being included as part of the JS bundle, which is the primary goal here. Later on, it would be nice to stand up a CDN layer or some other type of optimized hosting, but we can add that separately later.
From looking at the code, it seems groundstation layers are already loaded correctly; I'll use it as a rough guide for NSO.
Mostly true. I'd like to refactor that code to use async-await
for cleaner async handling, and remove the "backup data" dependency, but that's generally the approach to use.
Not sure what you mean by this exactly - still trying to piece together what this file stores exactly besides the boundary polygons. If some edit is needed in this file, a deeper insight into what you mean would be great, then I'll add to our game-plan if needed :)
I just meant that admin_boundries
is used by several different layers, so we have to make the decision whether to specify it individually per layer (which would modify the layer config schema), or to have it be a separate path that gets configured with the app instance and asynchronously loaded once, globally. No changes needed to the actual json file itself.
I think it probably makes sense to add it as a new layer type that gets loaded asynchronously and other layers depend on it.
Great! I think I have a good vision now on how to approach this issue.
Mostly true. I'd like to refactor that code to use async-await for cleaner async handling, and remove the "backup data" dependency, but that's generally the approach to use.
I'll include this in my PR.
I think it probably makes sense to add it as a new layer type that gets loaded asynchronously and other layers depend on it.
I'll take a shot at this as well. I'm assuming by
new layer type
you mean to add as a layer in layers.json?
One thing I'm worried about however is that converting boundaries to a new layer type in layers.json
will open the door to allow multiple boundary layers to be provided, which could break something down the line since its essentially a singleton dependency.
I'll go with your approach for now with added error checking to ensure there's only one definition
One thing I'm worried about however is that converting boundaries to a new layer type in
layers.json
will open the door to allow multiple boundary layers to be provided, which could break something down the line since its essentially a singleton dependency.I'll go with your approach for now with added error checking to ensure there's only one definition
Ya, that's a good point! I think you can just add it as another validation check in src/config/utils.ts.
Can't wait to see it!
Right now JSON data files (e.g. admin boundaries, NSO files) are being directly imported into the code base (e.g. using an
import
statement in JS). This means that the full JSON data gets bundled into our javascript code, which is pretty seriously bloating the deployment.These files should be hosted somewhere (e.g. S3 buckets) and fetched asynchronously instead.