Refactor using React Leaflet and React best practices

[rod-glover]

Overview

Resolves #112

This is a big one! Apologies; I intended to break this down into several incremental PRs. But as noted in the associated issue, the old code and React Leaflet-based solutions turned out not to play well together, i.e., not at all. This forced a fully componentized refactoring, rather than a hybrid approach.

Isolation of changes

This branch replaces MapController and its descendants with refactored code.

• All significant changes are contained within MapContoller and its child components and supporting code.
• A few minor changes to parent components have been make (e.g., redefinition of area).
• The original MapController and its co-conspirator CanadaMap have been removed and replaced by the new MapController and its cast of new supporting components.

Questions and suggestions for reviewers

I think it might make sense to review more or less top-down, starting with AppController / DualController / MotiController, which render MapController, which contains the big changes.

AppController / DualController / MotiController

1. Note refactoring of area to be
   1. state only at this level (via AppMixin), prop everywhere else
   2. represented as geojson and converted to WKT only when passed to DataController

MapController

1. Mostly refactors the content of original MapController and CanadaMap into React-Leaflet and React components that are composed here (user-selection components rendered as children of DataMap).
2. Most of it is devoted to "wiring up" the rendered components. Except, unfortunately, for ...
3. Preserves some less desirable coding from original code in loadMap and componentWillReceiveProps. I'm preparing some questions and suggestions on refactoring this properly but I think there are enough changes already that this should be left for a follow-up PR. Essence of the questions/suggestions:
   1. Why is variable_id (and comparand_id) extracted from metadata instead of passed in as props?
   2. Don't store values that are immediately derived on state. Just compute them and use them in place.
4. I think the business of representing the choice of a dataset as an encoding of it can now be simply replaced by creating the list dataset and having the selector return the index into it. But this is part of the cleanup of loadMap and componentWillReceiveProps.

DataMap

1. Mostly just wires up the React-Leaflet components needed to render the "data map", which is the map with the raster and isoline layers. This is its primary purpose.
2. Renders its children to enable composing at higher level.
3. NcWMSColorbarControls and NcWMSAutosetColorscaleControl controls are dependent on their sibling DataLayers. Since they are rendered as sibling components (and can't be otherwise, without rewriting the React Leaflet WMSTileLayer component), their existence relative to each other is asynchronous, and that asynchrony has to be (or at least is most simply) coordinated by storing refs to the DataLayers on state.
4. Suggest promoting the area controls and layer up to AltMapController? It doesn't do anything at this level, and isn't directly concerned with DataMap's primary purpose, which is the data map. That would make this a really crisp component.

CanadaBaseMap

1. Provides the base layer and renders all children (React Leaflet components) within itself.
2. Simple, once you understand how to invoke L.Proj.CRS for our kind of tile layers. See the comments to that.
3. Suggest crs, version, srs, origin not be props, just constants. I can't see how they can be useful as props. Am I missing something?

DataLayer

1. Simple, but might benefit from some minor refactoring (TODO).

NcWMSColorbarControl

1. Demonstrates how to create a new React Leaflet control component. Agreeably simple.

LeafletNcWMSColorbarControl

1. Custom pure Leaflet control that does the heavy lifting.
2. Could probably be optimized a bit so it doesn't re-render the control every time. But it has to render and empty control when its layer argument is null.
3. Would be cleaner to render React components instead of building content with L.DomUtil.

NcWMSAutosetColorscaleControl

1. Ditto.

LeafletNcWMSAutosetColorscaleControl

1. Ditto.

src/HOCs/compAcontrolsB

1. A HOC that encapsulates and DRYs up the state and callback wiring necessary to have one component "control" another, e.g., a Button open a Modal.
2. Used to create the button-triggered map control dialogs.

MapSettings

1. Created by HOC compAcontrolsB. Button opens dialog.

DatasetSelector

1. Extracts dataset selection for MapSettings.
2. Needs refactoring: See TODO.

MapSettingsDialog

1. Mostly wiring of the components that render the required selectors etc. inside the dialog.

DataDisplayControls

1. DRYs up the creation of selectors in MapSettings for both raster and isoline layers.

TimeLinkButton

1. Button with tooltip for linking/unlinking time selectors in MapSettings

TimeSelector

1. Extracts time selector component of MapSettings.
2. A bit of a code smell here (see TODO).

PaletteSelector

1. Extracts colour palette selector component of MapSettings.

ScaleSelector

1. Extracts scale (linear/log) selector component of MapSettings.

GeoLoader

1. Created by HOC compAcontrolsB. Button opens main GeoLoader dialog.

GeoLoaderDialogWithError

1. Created by HOC compAcontrolsB. Error on loading opens error dialog.

GeoExporter

1. Created by HOC compAcontrolsB. Button opens GeoExporter dialog.

MapFooter

1. Extracts map footer component of map.

Known issues

• Autoscaling and colorbars interact in an undesirable way. After clicking AS, the colorbars don't update properly when model, scenario, or variable is changed. This seems to be mediated by the use of the layer parameter colorscalerange. What's really needed is a clear spec of the desired behaviour.
• Officially, react-loader is not compatible with React 16. npm install emits warnings. But it works fine. No actual issue here.
• React Bootstrap is not fully compatible with React 16. According to some RB documentation, there aren't many serious problems, and many people use it with React 16 anyway. We're doing this. An issue that might have affected us (Modals not working properly) has either been fixed. No actual issue here.

Principles and best practices followed

1. Favor declarative code (i.e. results rendered in JSX) over procedural code.
   1. Rendering components is declarative. Almost everything can be done with components.
   2. Conversely, code that procedurally builds or modifies components indicates that the breakdown into components is incomplete.
2. Break down large components into many smaller, single-purpose components that are composed by a parent component.
   1. Limit the purpose and complexity of each component. One component, one responsibility. (The responsibility of components higher in the hierarchy is mainly to compose other components.)
   2. Encapsulate details. Don't let information spread beyond exactly where its needed.
   3. Maximize code locality. Supporting code (e.g., a callback) and its user (e.g., a component) should very close to each other, uncluttered by code that supports other things.
   4. Let React do the work of coordinating actions. Small, properly composed components need little in the way of lifecycle methods. The few cases where lifecycle methods need to be defined are simple and focused.
3. Use state and props correctly.
   1. A value should never be both state and prop. If you are tempted to do so, promote the state upward, pass it in as a prop, and add a callback to cause state modifications.
   2. A value shared by components should be the state of exactly one component (the lowest common ancestor of all components that share it).
   3. All other components receive this value as a prop.
4. Extract things like data services into separate modules.
   1. This isolates the complexity of building requests and their dependence on specific tools (e.g., axios) and APIs, and promotes reuse and DRYness when building other components that need the same or similar requests.
   2. This is a far superior substitute for mixins supplying the same functions.
   3. The same principle applies to other computations or services that may be shared between components.
5. Declare props and state explicitly so that it is easy to understand what information the component depends on.
   1. Declare every* prop in propTypes. (*Except if you are passing through gobs of options to React Bootstrap components. Don't declare those ones unless you are feeling masochistic.)
   2. Initialize every* state variable in constructor. (*No exceptions. Initialize to null or undefined if necessary.)
   3. Comment props and state declarations if it's not obvious what they're for.

The end result of this is a kind of splitting of components into two types, with little mixing of the qualities of the two.

1. Components higher up the hierarchy are mainly responsible for "wiring up" their children with the right connections of props and state. They tend to have more lines of code, but it is mostly simple code devoted to the "wiring".
2. Components lower in the hierarchy provide a single specialized functionality, e.g. a selector, a map layer. These components have fewer lines of code, but it may be more complex because it is mostly devoted to functionality, not wiring.

Note: "Wiring up" state and props is tedious, bulky, and repetitive. Which is why tools like Flux and Redux were invented. But we should stick with a bit of tediousness for now. Those tools come with their own complexity and tediousness.

Overview of the new componentization

The following is a compact distillation of the new components:

AppController / DualController:
    state:
        meta
        comparandMeta  // in DualController
        area

    render:
        ...
        MapController | AltMapController  // controlled by env. variable
        ...


AltMapContoller:
    props:
        meta: PropTypes.array,
        comparandMeta: PropTypes.array,
        area: PropTypes.object,
        onSetArea: PropTypes.func
     
    state:
      run: undefined,
      start_date: undefined,
      end_date: undefined,

      variable: undefined,
      variableTimes: undefined,
      variableTimeIdx: undefined,
      variableWmsTime: undefined,

      comparand: undefined,
      comparandTimes: undefined,
      comparandTimeIdx: undefined,
      comparandWmsTime: undefined,

      rasterLogscale: 'false',
      rasterPalette: this.props.comparandMeta ? 'seq-Greys' : 'x-Occam',

      isolineLogscale: 'false',
      isolinePalette: 'x-Occam',
      numberOfContours: 10,

      rasterRange: {},
      isolineRange: {},

    render:
        DataMap
            StaticControl
                GeoLoader
            StaticControl
                GeoExporter.Modal
            StaticControl
                MapSettings
            StaticControl
                MapFooter


DataMap:
    props:
        rasterDataset: PropTypes.string,
        rasterVariable: PropTypes.string,
        rasterTime: PropTypes.string,
        rasterPalette: PropTypes.string,
        rasterLogscale: PropTypes.string, // arg for ncwms: 'true' | 'false'
        rasterRange: PropTypes.object,
        onChangeRasterRange: PropTypes.func.isRequired,
    
        isolineDataset: PropTypes.string,
        isolineVariable: PropTypes.string,
        isolineTime: PropTypes.string,
        isolinePalette: PropTypes.string,
        numberOfContours: PropTypes.number,
        isolineLogscale: PropTypes.string, // arg for ncwms: 'true' | 'false'
        isolineRange: PropTypes.object,
        onChangeIsolineRange: PropTypes.func.isRequired,
    
        area: PropTypes.object,
        onSetArea: PropTypes.func.isRequired,
        
    state:
      // set by ref callbacks so that colorbars and autoscaler can coordinate
      // on them after layers are created
      rasterLayer: null,
      isolineLayer: null,
    
    render:
        CanadaBaseMap
            DataLayer   // raster
            DataLayer   // isoline
            NcWMSColorbarControl   // raster
            NcWMSAutosetColorscaleControl
            NcWMSColorbarControl   // isoline
            FeatureGroup  // (react-leaflet)
                EditControl // (react-leaflet-draw)
            {props.area && GeoJson}  // "area" layer (react-leaflet)
            {children}


CanadaBaseMap:
    render:
        Map // (react-leaflet)
            TileLayer // base map (react-leaflet)  // TODO: ? extract as CanadaBaseLayer
            {children}


DataLayer:
    props:
        layerType: PropTypes.string, // 'raster' | 'isoline'
        dataset: PropTypes.string,
        variable: PropTypes.string,
        time: PropTypes.string,
        palette: PropTypes.string,
        logscale: PropTypes.string,
        range: PropTypes.object,
        onLayerRef: PropTypes.func,
        onChangeRange: PropTypes.func.isRequired,

    render:
        // special behaviour to facilitate coordination with sibling controls
        if props.dataset:
            return WMSTileLayer
        else:
            onLayerRef(null)
            return null


NcWMSColorbarControl:
    props:
        layer
        options
        // unused except to force updates on changes
        dataset
        variable
        time
        palette
        logscale
        
    render:
        // wraps a Leaflet control in a React component


NcWMSAutosetColorscaleControl
    props:
        layers
        options
    
    render:
        // wraps a Leaflet control in a React component
    

GeoLoader:
    A:
        render:
            Button
                ...

    B:
        render:
            DialogWithError

    ButtonWithModal = compAcontrolsB(A, B)

    render:
        ButtonWithModal


DialogWithError:
    A:  // main dialog
        render:
            Modal
                ...

    B:  // error dialog
        render:
            Modal
                ...

    ModalControlsModal = compAcontrolsB(A, B)

    render:
        ModalControlsModal



GeoExporter.Modal:
    A:
        render:
            Button
                ...
    B:
        render:
            Modal
                ...

    ButtonWithModal = compAcontrolsB(A, B)

    render:
        ButtonWithModal


MapSettings:
    props:
        title: PropTypes.string,
        meta: PropTypes.array,
        comparandMeta: PropTypes.array,
    
        dataset: PropTypes.string,  // current dataset selection, encoded as JSON string
        onDatasetChange: PropTypes.func.isRequired,  // callback, arg is enocded JSON string
    
        variableTimes: PropTypes.object,
        variableTimeIdx: PropTypes.string,
        onChangeVariableTime: PropTypes.func.isRequired,
    
        hasComparand: PropTypes.bool,
        comparandTimes: PropTypes.object,
        comparandTimeIdx: PropTypes.string,
        onChangeComparandTime: PropTypes.func.isRequired, // required???
    
        timesLinkable: PropTypes.bool,
    
        rasterPalette: PropTypes.string,
        onChangeRasterPalette: PropTypes.func.isRequired,
    
        isolinePalette: PropTypes.string,
        onChangeIsolinePalette: PropTypes.func.isRequired, // required???
    
        rasterLayerMin: PropTypes.number,
        rasterLogscale: PropTypes.string,
        onChangeRasterScale: PropTypes.func.isRequired,
    
        isolineLayerMin: PropTypes.number,
        isolineLogscale: PropTypes.string,
        onChangeIsolineScale: PropTypes.func.isRequired,
    
    state:
      linkTimes: this.props.timesLinkable,
    
    MapSettingsButton:
        render:
            Button
                ...

    MapSettingsDialog:
        render:
            Modal
                DatasetSelector
                DataDisplayControls  // raster
                {props.hasComparand && LinkControls}
                {props.hasComparand && DataDisplayControls}  // isoline

    ButtonWithModal = compAcontrolsB(MapSettingsButton, MapSettingsDialog)

    render:
        ButtonWithModal


DatasetSelector:
    render:
        Selector  // datasets


DataDisplayControls:
    render:
        div
            TimeSelector
            PaletteSelector
            ScaleSelector


LinkControls:
    render:
        Button
            ...

MapFooter:
    render:
        StaticControl
            h5
                ...


StaticControl:
    // custom react-leaflet control that renders its React children inside
    // a leaflet Control element

[rod-glover]

Oops: ButtonWithModal = buttonWithModal(this.A, this.B) --> AcontrollingB = compAcontrolsB(this.A, this.B)

[rod-glover]

Undo: Outdated test harness

[rod-glover]

As you say, @jameshiebert , there is still a way to go on the refactoring; MapController in particular. I'd like every component to be quite simple and small.

[corviday]

Testing it now. It looks really great! I'm only finding tiny things, some of which probably predate this refactor.

• Time Of Year Selector for Long Term Average graph in DataController doesn't display currently selected value; just needs passed a value prop, I think.
• Time Of Year Selector for Long Term Average graph in DualDataContoller doesn't display currently selected value. value prop?
• Still a few console.log() calls kicking around, it looks like.
• Not sure if this is a pre-existing bug, but there are no options in the "Dataset" selector on the Annual Cycle Graph in the /compare portal. It doesn't seem very related to the refactor.

Also found an intermittent Dual Controller bug related switching ensembles and querying the data API, but it was not introduced by this refactor and I put it in its own PR. So if anyone is testing and gets an error message about data not being available on the /compare portal's Long Term Average Graph, that's why.

[rod-glover]

@corviday thanks for the testing and review. The problems you noted are pre-existing; see the current production deploy. Also, my changes are confined to MapController subtree, so it is highly unlikely I modified any behaviour of the graphing side of things.