search

osmlab / show-me-the-way

See OSM edits happen in real time.
https://osmlab.github.io/show-me-the-way/
MIT License
130 stars 46 forks source link

Display nodes that have tags #57

Closed mstock closed 6 years ago

mstock commented 6 years ago

Issue #56 requested that nodes should also be displayed. The suggested changes in this pull request add this functionality by placing a marker for nodes that have been created, modified or deleted if the node has any tags. Since this requires that tags are also part of the data structure that is provided by osm-stream, it currently has no visible effect though, since tag parsing for nodes only becomes available if/when osmlab/osm-stream#13 (or something similar) gets merged, which currently has a syntax error that could be fixed by applying a trivial patch like the one in mstock/osm-stream@56bf6bd5158ade7eb0fad83d48c746156e4e833e. Once that pull request is merged, somebody would have to cut a release of osm-stream and then the corresponding dependency in show-me-the-way would need updating. The first five commits in this pull request are mainly smaller code cleanups and fixes - I can extract them to a different branch and create a separate pull request or even drop them if you prefer. For demonstration purposes, I've deployed a build with the node feature that uses a patched version of osm-stream on my Github page.

iandees commented 6 years ago

@mstock thanks for working on this! I tagged a version of osm-stream with the fix for more than ways as 0.0.14. Can you update the package.json here so it uses it, then we can try it out?

mstock commented 6 years ago

You're welcome :). I just pushed another commit which updates osm-stream to osmlab/osm-stream#v0.0.14. Since there does not seem to be a release on NPM, yet, I assume using the Github reference in package.json is the right way to do this.

iandees commented 6 years ago

Yea, that probably will be ok for now. I'm not sure I have access to the npm project 🤔

iandees commented 6 years ago

This looks great, thanks!

sfkeller commented 6 years ago

Thats very cool! Thanks Manfred. Ian: Is it already deployed on osmlab.github.io/show-me-the-way/#bounds=34.67,-11.78,70.55,31.73 ?

iandees commented 6 years ago

Yep, it should be.

sfkeller commented 6 years ago

Thanks. It's hard to validate from outside; I'm still waiting to see a node popping up :-).

mstock commented 6 years ago

Actually, I don't think it is - the GitHub page uses the 'compiled' bundle.js, and that has not been touched for a few months. I did not include a 'compiled' bundle.js in my pull request, so somebody still has to do that and commit/push the 'compiled' file. I'm not sure what the right or best workflow is, but I tend to think that it is better to not include a 'compiled' version in pull requests, since this will cause conflicts if multiple pull requests get merged at the same time, as they would all contain a different variant of the 'compiled' file (and to get all the new things, a rebuild after the merges would be required anyway). It could also be abused to smuggle in unwanted stuff (even though the 'compiled' file is not unreadable, there still can be a massive diff that had to be reviewed to be somewhat safe) or it could be out of sync with the rest of the pull request, and it's also not clear if it still can be built on another machine if it is only part of the pull request and not (re)built by a maintainer. Btw.: I think #56 can be closed once bundle.js got built and published.

iandees commented 6 years ago

Ah, you're right! Thanks for looking into it. It's been a while since I've made a change on this repo 😄.

I'll re-build bundle.js and push it up for now. In the longer term we could switch over to a replacement for GitHub Pages that will build the bundle.js for us.

iandees commented 6 years ago

I pushed up the updated bundle, but I haven't seen a node show up yet. Let me know if you find one.

mstock commented 6 years ago

I took a look at the 'compiled' bundle.js, and it seems like the build did not include osm-stream 0.0.14 as specified in package.json (but an older version instead), so I guess that your node_modules directory still contained an older version. This was likely either caused by npm install somehow failing to download this version (the syntax I used to specify the version seems to require at least npm 1.1.65), or not running npm install at all before building the bundle.

In order to automate the build with GitHub pages, one could probably use TravisCI - I've never tried this though, and since it requires a personal token, TravisCI will likely get write access to all repositories the creator of the token has access to (so I'd try to create a second GitHub user for this, and only grant this user access to those repositories where this is required - I don't know if this is actually supported though), and it might also be a good idea to start doing development on a different (like master) and protected branch and 'reserve' the gh-pages branch for the build results from TravisCI.

iandees commented 6 years ago

I pushed the updated bundle.js and am seeing nodes on the deployed site. Looks good!

This highlights how much is happening in OSM, and that this tool might need to group these changes together so that we spend less time jumping around the world.