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.

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

Rewrite the web app in Svelte #219

Merged
merged 8 commits into from
Jul 11, 2023
Merged

Rewrite the web app in Svelte #219

merged 8 commits into from
Jul 11, 2023

Conversation

dabreegster
Copy link
Contributor

Street explorer and the lane editor have thus far been written in framework-less JS. I've found it rather painful as the web apps have grown, and have felt slowed down when trying to rapidly experiment with new ideas (thickening road center lines until they hit buildings, live-tuning parameters for edge costs in pathfinding, extracting POIs from OSM). I've become quite comfortable in Svelte, TS, and MapLibre over the past months from work projects. In the background, I've slowly been exploring rewriting the osm2streets apps using this stack in https://github.com/dabreegster/osm2svelte. This PR is finally bringing the results of that experiment over here, replacing the old JS app!

Existing JS app: https://a-b-street.github.io/osm2streets
New Svelte app: https://dabreegster.github.io/osm2svelte (I'll change the github workflows to deploy here once this PR is merged)

Quick demo of the street explorer:

st_expl.mp4

And the lane editor:

editor.mp4

Big changes

  • Change from Leaflet to MapLibre GL. I'm much more familiar with the latter from work. The API for working with MapLibre is harder than Leaflet -- attaching popups or changing opacity for a hover effect as some examples -- but getting Leaflet to work nicely with Svelte was a bit of a challenge anyway. The osm2streets-svelte library has some opinionated helpers like Layer.svelte that make the end-user API quite nice.
  • The UI is more reactive. The user initially sees generated layers; there's no "Generate details" button. After changing import settings, the change happens immediately.
  • The road ordering debug layer is now shown with popups per intersection, rather than for everywhere

There are a few unimplemented features from the previous version, notably debug layers. I'll file an issue with those tasks, but I don't want to block cutting over to this on that, because small iterations are easier once I'm using the new UI day-to-day and am motivated to improve it. :)

A common library

I'm going to keep the osm2svelte repo around and use it for my wild experiments. After this PR is merged, I'm going to delete dupe code from there and depend on things here by npm github dependencies. My intention is to later split out the osm2streets-svelte subdirectory as a standalone NPM module, document it better, and encourage people to build their own web apps making use of it.

Reviewing

Happy for a real code review, but I recognize this is absolutely massive. @BudgieInWA, if you're happy with the overall tech choices, that's the main thing I'm looking for.

CCing @andrewphilipsmith for osm2streets / A/B Street context. When we start "lifting" things like the turn restriction code to happen at the osm2streets layer, this is a place for us to interactively debug and make simple UIs for diff testing. npm run wasm && npm run dev is orders of magnitude faster than rebuilding A/B Street for tiny changes in one base crate, because we don't need to link to dozens of dependencies to effectively reimplement a web browser. :)

And CCing @yongrenjie if there's any interest in discussing Svelte style. I've made some choices here I'm happy with, but other things (ordering and reactive statements) are very awkward.

I'll leave a bunch of comments on the code to try and intro some Svelte and explain weird choices.

@dabreegster dabreegster requested a review from BudgieInWA July 9, 2023 16:03
Copy link
Contributor Author

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

#220 for all the later improvements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add this back after this PR is merged. Otherwise the current app gets replaced immediately and comparison is harder

- Street Explorer: import OSM data from Overpass or test cases, visualize and debug the output
- Lane editor: modify OSM way tags and visualize the results, to make editing lane tags easier

## Installation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set up instructions are here

"preview": "vite preview",
"check": "svelte-check --tsconfig ./tsconfig.json",
"fmt": "npx prettier --write *.html src/**",
"list-tests": "./public/list_tests.sh"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to manually run this when adding new tests. There could still be nicer solutions here...

});
</script>

<Layout>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Svelte is component based, so we can very naturally express the overall organization of the app and share components so much more easily than with vanilla JS

},
};

$: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://svelte.dev/tutorial/reactive-statements

This runs whenever anything referenced inside of here changes. In this case, $clickedLane. (The $ is the syntax for accessing a store: https://svelte.dev/tutorial/auto-subscriptions)

@@ -0,0 +1,82 @@
<script lang="ts">
export let settings = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note we don't have to do anything to detect changes to the form and propagate. bind:value makes it a two-way to this prop here, and then since the caller does bind: as well, they can then use reactive statements and listen for changes. https://svelte.dev/tutorial/component-bindings


let gj: GeoJSON | undefined = undefined;
let show = true;
$: if ($network && $hoveredIntersection) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the layers are reactive, changing usually when the loaded StreetNetwork does. And in this case, based on hover state

@@ -0,0 +1,75 @@
<script lang="ts">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two osm_input components don't have any concept of StreetNetwork or osm2streets. I have some experiments where I want to process OSM data completely differently, but still conveniently extract from overpass or use something built-in

return beforeId;
}

// Later entries are drawn on top
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will need rethinking later, if this stays part of the common library meant to be used by multiple apps

"checkJs": true,
"isolatedModules": true,
"types": ["vite/client"],
"strict": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to turn this on eventually, but the tail is very long for fixing every problem...

@BudgieInWA
Copy link
Collaborator

After only reading through the PR description, I am happy to see this, and happy for it to merge! I'm going to keep this PR around to look back at later.

@dabreegster
Copy link
Contributor Author

Merging, then will fix up GH actions. (Hopefully in one commit, but if it turns into whack-a-mole with bugs, I'll switch to a tmp branch)

@dabreegster dabreegster merged commit ef97f6a into main Jul 11, 2023
@dabreegster dabreegster deleted the osm2svelte branch July 11, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants