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

Core refactor #961

Merged
merged 44 commits into from
Jun 20, 2023
Merged

Core refactor #961

merged 44 commits into from
Jun 20, 2023

Conversation

bhousel
Copy link
Contributor

@bhousel bhousel commented Jun 20, 2023

Opening a PR here for visibility, but I'm ready to merge the last month's work that reorganizes and modernizes most of the core components of Rapid to make things a lot easier to work with, and hopefully eliminate a lot of the "magic" in the project that was making startup complicated and reduce the amount of spaghetti code.

I'm drawing inspiration from how Pixi.js organizes their core components into "systems" owned by a "renderer".
In Rapid, our "systems" will continue to be owned by our "context" object.

The deets:

  • Core components are now "systems". Each system has one job.
  • All code is now written in modern ES6+ style Javascript using classes.
  • No more globals (singletons, whatever we called them). Everything must be accessed through Context.
  • Each component has a constructor that receives the Context. Constructors may not use things from other components.
  • Each component has dedicated init and reset methods. We had these before but we're making it more clear now. Init is when you can start referring to things in other components. (We should make the inits async).
  • All these components use EventEmitter to emit events - no more d3-dispatch/d3-rebind
  • Trying to avoid the old d3-style setter/getter methods in favor of just using ES6 class properties.
  • The naming of a lot of things has changed for consistency and clarity.
    • prefer ThingCategory (e.g. MoveMode or MapillaryService or ValidationSystem)
  • A few legacy components got updated to ES6 classes - we didn't have time to get these in before the v2 launch.
  • These rules apply to everything in core of Rapid.. Modes, Behaviors, Services, Systems.
  • Components can be replaced before calling Context.init - so if someone really wanted to replace a core component with their own, it's now possible.

todo:

  • Some more things should be systems:
    • The scene graph should maybe a system. It's currently owned by the PixiRenderer. But other parts of the code talk to it to know what layers are enabled, what things are selected or hovered, etc. It might need a new name too.
    • utilKeybinding() should be KeyboardSystem or something. (Different parts of the code should not be responsible for their keyboard shortcuts, and this would be a first step towards letting users remap their keyboard shortcuts).
    • We need a SchedulerSystem - we are currently using the Pixi.Ticker event loop now, but we also sometimes need to schedule background work, and Rapid performs poorly on Safari because of the polyfill for RequestIdleCallback. Also should start moving things to web workers.
    • I'd like a proper DatabaseSystem that wraps IndexedDB and is useful for larger amounts of data that can be accessed async and in workers. We're currently abusing the StorageSystem (wraps localStorage) to store the user's edit history and it has hit its limits.
  • Validators are still mostly function modules, we will modernize these as time permits, and with an eye for getting them running in workers.
  • Some consideration needs to be made for which parts of Rapid are still OSM-only and how to make things more generic. For example many of our third party datasets (like the roads or buildings) work by making fake OSM features, and we still have different modes like SelectThing and SelectOSMThing - We can't select multiple things at the same time, and this is blocking us from shipping cool features like "merge data from dataset into OSM". (This is just one example of how the the OSM-focus of the old architecture limits us). Of the top of my head, the sidebar, the info panels, etc needs to be updated.
  • Really the whole UI needs a refresh. They are mostly still d3-style function components
  • Init is still not a solved problem. All init methods could probably be async so components can depend on other components cleanly.
  • Documentation. I've done so much, but there's so much more to do 😅

Trying to get to a place where all objects are just owned by context.
PixiJS uses this "system" naming convention so we will too
Trying to get to a place where all objects are just owned by context.

This includes quite a lot of changes to the preset/field system:
- All the preset related components are ES6 classes now
- Stuff moved from modules/presets -> modules/core
- Generally, tried to simplify the code as much as possible and avoid "magic"
- Lots of changes around Collection, now it just contains an `array` property,
  no more pretending Collections is an Array or PresetIndex is a Collection
- Some things that were functions before (setAddablePresetIDs) are just properties now
- uiField -> UiField is now an ES6 class too
- Most of the fields have been rewritten for ES6, many revisions and updates
- The old way of converting a field -> presetfield -> uifield was pretty
  complicated, now I try to explicitly copy over properties from one
  component to another and call out places where a class property gets
  replaced by a class function

Also fixed an unrelated bug in uiIcon / disclosure that was causing the hide/show
toggle icons to not appear (because I removed third argument to UiIcon in 05f2f34)
All will move to ES6 classes that have reference back to context
and follow predictable setup and initialization flow.
No more magic contextless globals.
Also removing maprules - I dont think anyone uses it anymore.
Fixed the missing links to the preset and location systems
now that NSI service has a reference back to context
Still need to fix the show/hide viewer code
This is all of them!  Still need to restore some tests
Mostly this is just using fat arrow functions to preserve `this`
Rapid does work, but now the tests are a mess.

This is because the interactions between the localizer and everything else get
complicated, as creating a context will start loading data and the fileFetcher
isn't global anymore.  The tests did have a lot of code in spec_helpers to
prevent files from being fetched, that doesn't work now.

This can start getting easier after we change the localizer similarly.
This is a big change:
- Eliminated the global `localizer` and `t()` functions
- Added `context.t()` and `context.tHtml()` convenience functions
- Some util functions relied on the global `t()` for localization
  (e.g. utilDisplayName, etc) these are moved into the LocalizationSystem now
- Some more things need context now to avoid using the global `t()`
  - `ImagerySource` now passed a context
  - `ActionMergeRemoteChanges` rewritten to take an options param with the localizer
  - several more UI classes take context now
  - Reordered some arglists (e.g. for uiPane), context should always be the first one
- Lots of changes to the tests.  They were loading the general en locale
  strings before and they should not do this.
- Still some more work to do on tests to be better at mocking context
- Restored the service tests that I didn't do before
  (Osm, Kartaview, Mapillary, Streetside)
- Fixed the other service tests that were failing because of missing fetch mock resets
- Removed the sticky fetch mock routes (they were only needed by serviceosm capabilities test)
- Also went through and simplified a bunch of the validator tests
  - We don't need to create a full context for these
  - We don't need to run them async to wait for data to load
This commit also moves a bunch of stuff from /core -> /core/lib for organization
I also got the old RendererMap tests working again.
These were skipped, but they can work now with some substantial mocking.
Mostly because MapSystem listens to events emitted by many other things.

It might make more sense to avoid these listeners and instead have the other
systems tell them map to redraw as needed. We have map.deferredRedraw() now
so this can be done in a less intrusive way.

Also, we still have to sort out initialization of the map a bit better, but
we're getting there.
Following up on an idea from 1d5e617,
Just call `mapSystem.deferredRedraw()` or `mapSystem.immediateRedraw()`
instead of listening to events from various places.  This much simplifies the
various Pixi layers that were registering listeners on the service objects.
It also makes mocking mapSystem a little easier.
This is a big change but will make the code easier to work with.
Some linger issues:
- Switching from d3_dispatch to EventEmitter means we have to change some of
  the listeners in the walkthrough
- Some of the listeners seem to be firing too much
- `reset` used to be used for both resetting but also returning to a
  saved checkpoint.  I split these into two functions.
- It used to dispatch a 'reset' event that was only used by the validator.
  The validator also has 2 resets, one that resets everything and one that
  preserves the user's ignored issues.  This is also something we can clean up.
- It's pointless to requestIdleCallback the osm api calls that will be async anyway
- We're at the point where we can get rid of the distinction between
  instantiate and initialize in the `.init()` function

All core classes can be instantiated earlier now and in any order, as they
have class constructors that don't depend on each other for anything.
By using smaller chunks, we will block the main cpu for shorter amounts of
time, allowing the browser to be more responsive.

Eventually I'd like to move this to a worker, but we don't have a great way
of sharing the context/graph/edits with worker processes yet.
e.g. "MoveMode" not "ModeMove"
The former is more how we would speak about and think about the thing.
We already have the core systems named this way (e.g. "EditSystem")
so we may as well do it for all the things.
Behaviors -> AbstractBehavior -> EventEmitter
Modes -> AbstractMode -> EventEmitter
Services -> AbstractService -> EventEmitter
Systems -> AbstractSystem -> EventEmitter
This would allow a custom install of Rapid to add/replace/remove
these components before context.init() is called.  Maybe an early
first step towards supporting plugins and other customization.
Also replace the d3-dispatch('enter','exit') with EventEmitter and just emit 'modechange'
I thought about making these static.
For modes it's easier for them to be class properties than static properties.
 (i.e. we need to get `mode.id` in several places)
I'd still rather not have 2 select modes, but at least the modes are more
standardized now.
- context, ui, validations all had a lot of work to do
  (many tests that were skipped before will now run!)
- We avoid creating a new Context() now just to get unit tests to work
  Instead, mock out the systems that the unit tests need.
localStorage can only store strings, so before if we passed it a null, it stored 'null'.
Then on a restart we'd see the 'null' there and think there were changes to restrore.
@bhousel bhousel merged commit 2b55dd5 into main Jun 20, 2023
bhousel added a commit that referenced this pull request Sep 20, 2023
(followup from ca9be60, also see #961)

Continuing to remove editing and localization operations from Context.
The idea here is to remove "magic" from the code.
These functions were just redirects from the Context to the EditSystem,
so using them obscured what was really going on.  It was easy to get
confused about "which graph" the code was really using.

- Removed `context.graph`, `.entity`, `.hasEntity`
  Code must go through the EditSystem to do these things now.

- Convert several EditSystem accessor methods to ES6 getters
  - `.graph()`  - removed
  - `.base`     - now a property
  - `.current`  - added
  - `.current` and `.base` now return the `Edit`, which has `.graph` as a property.
    This makes it easier to compare the current and base graph, and just more consistent flow.

- Also WIP on replacing `context.t` and `.tHtml`
  Code to do these things should go through the LocalizationSystem
  There is quite a lot of this, and it will take a while.
bhousel added a commit that referenced this pull request Sep 21, 2023
(followup from ca9be60, 43e7f89, also see #961)

Continuing to remove localization operations from Context.
The idea here is to remove "magic" from the code.

- Removed `context.t`, `.tHtml`, `.tAppend`
  Code must go through the LocalizationSystem to do these things now.
bhousel added a commit that referenced this pull request Oct 6, 2023
(followup from ca9be60, also see #961)

Continuing to remove editing and localization operations from Context.
The idea here is to remove "magic" from the code.
These functions were just redirects from the Context to the EditSystem,
so using them obscured what was really going on.  It was easy to get
confused about "which graph" the code was really using.

- Removed `context.graph`, `.entity`, `.hasEntity`
  Code must go through the EditSystem to do these things now.

- Convert several EditSystem accessor methods to ES6 getters
  - `.graph()`  - removed
  - `.base`     - now a property
  - `.current`  - added
  - `.current` and `.base` now return the `Edit`, which has `.graph` as a property.
    This makes it easier to compare the current and base graph, and just more consistent flow.

- Also WIP on replacing `context.t` and `.tHtml`
  Code to do these things should go through the LocalizationSystem
  There is quite a lot of this, and it will take a while.
bhousel added a commit that referenced this pull request Oct 6, 2023
(followup from ca9be60, 43e7f89, also see #961)

Continuing to remove localization operations from Context.
The idea here is to remove "magic" from the code.

- Removed `context.t`, `.tHtml`, `.tAppend`
  Code must go through the LocalizationSystem to do these things now.
@bhousel bhousel deleted the core_refactor branch November 8, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants