-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Switch geodata providers #7393
base: master
Are you sure you want to change the base?
Conversation
tasks/topojson/get_geodata.mjs
Outdated
import config from './config.json' assert { type: 'json' }; | ||
|
||
try { | ||
// Download data from UN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these origin URLs may change (link rot), I worry that this piece of the script could become out of date quickly. I'm not sure there's too much we can do here but I might recommend committing the current source file (but not adding it to the dist/
. Maybe someone else has a better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable. The script could be updated to look for the file first and only download if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marthacryan do you have an opinion or ideas on this? What's the ideal dev UX here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, in 3b47c67
I added the archive from the UN.
Co-authored-by: Nathan Drezner <38958867+ndrezn@users.noreply.github.com>
Q for @camdecoster : did the old |
Trying to get
This still produces error messages like the one shown below saying that the JSON files can't be found:
I've tried variations on the path in
Adding an extra |
@gvwilson yes the old maps had 'subunits' layers for a few regions (USA and Brazil did for sure). The UN maps don't include that information. Thanks for looking into the tests. I'm working on an update to get the tests working but I haven't pushed that yet. I'll try to get that out later today. |
@etpinard when you have a chance could you give this PR a look? Would love your eyes. Thank you! In the short term what was the source for US states/Canadian provinces in the old dataset? |
Refactor to simplify scripts Switch to UN/NE geodata hybrid Save UN geodata to archive Remove extra info from topojson Add centroids to geojson Use 'simplify' to create 110m maps
@@ -1,8 +1,27 @@ | |||
'use strict'; | |||
|
|||
var saneTopojson = require('sane-topojson'); | |||
const topojson = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any more concise syntax in js for achieving this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. This can be written more concisely to dynamically generate the file names or read the contents of a directory and use those to build the object, but that causes problems in the test suite. Karma runs in a browser context and therefore doesn't have the required Node libraries (like fs
and path
) without some changes to the config. Though it looks like a wall of text, it's simpler to just statically require everything. 🤷
Description
Adds scripts to build topojson from UN sourced data and updates build process to run these scripts.
Closes #7334
Changes
Testing
npm run build_topojson
and make sure the script completes successfullyNotes
TODO
draftlogs
:7393_feat.md
properties
info from final maps