Modularize Data Download #654

Benjaki2 · 2018-01-08T20:43:53Z

Changes in this pull request:

Modularize data download code
Adjust naming of map function exports to match universal styling
Modularize EDSC files
Include data download tests

* export as `dataHelper` instead of `L`

* all functions that were part of wv global are now exported * export reuseable constants `export const examples ='something'`

ZachTRice

Looks good. Tested locally and didn't find any errors. I do agree that the L naming is not named well / intuitive when looking at the code.

var L = {};
edsc.L = L;

ZachTRice · 2018-01-08T21:16:26Z

Actually, I am seeing the following error:

To reproduce to this link, click some granules to download, then switching projection.
https://localhost:3000/?p=geographic&l=VIIRS_SNPP_CorrectedReflectance_TrueColor(hidden),MODIS_Aqua_CorrectedReflectance_TrueColor(hidden),MODIS_Terra_CorrectedReflectance_TrueColor,VIIRS_SNPP_Fires_375m_Night(hidden),VIIRS_SNPP_Fires_375m_Day(hidden),MODIS_Fires_Aqua(hidden),MODIS_Fires_Terra,Reference_Labels,Reference_Features,Coastlines(hidden)&t=2015-01-02&z=1&v=-142.86539783640285,-126.84694971129696,110.54085216359715,94.49680028870304&ab=off&as=2008-01-02&ae=2015-01-02&av=3&al=false&download=MOD14

localjo

Great job modularizing a very complex section of code! I left a few comments about variable names that I think are confusing. My main point is that we should be choosing variable names that are consistent and clearly communicate what the code is doing with minimal mental overhead. I'm not asking for any changes, unless you agree with my comments or see an easy way to make things clearer.

localjo · 2018-01-08T20:51:53Z

web/js/data/data.js

@@ -0,0 +1,15 @@
+export function dataParser(state, errors, config) {


What do you think about naming this file parser.js?

A number of the 'parser' files in other features have other methods in them.

localjo · 2018-01-08T21:18:46Z

web/js/data/results.js

+import {dataHelper} from '../edsc';
+import {
+ CRS_WGS_84,
+ mapIsPolygonValid,


What's the difference between prefixing all of these with map and prefixing all of the Worldview JavaScript files with wv.? Does the extra prefix provide some value? Or are we being more verbose than we need to be?

I'd say we essentially got rid of thewv in wv.map.something and are left with the map.something as camel case. The prefix stands for the folder in which something is in. We can't just call a method parser, we need dataParser if we want to export unique names across the board. mapIsPolygonValid would be fine as isPolygonValid but I'm just trying to stick with a convention.

localjo · 2018-01-08T21:20:52Z

web/js/data/results.js

+import loEach from 'lodash/each';
+import {dataCmrRoundTime, dataCmrGeometry} from './cmr';
+import util from '../util/util';
+import {dataHelper} from '../edsc';


Why not rename the whole edsc directory to web/data/helpers or something and eliminate the edsc name completely?

localjo · 2018-01-08T21:25:36Z

web/js/edsc.js

@@ -1,9 +1,14 @@
+import edscArc from './edsc/arc';


What's the value of the edsc prefix? Didn't we agree that edsc isn't really relevant to this code anymore?

localjo · 2018-01-08T21:30:42Z

web/js/data/results.js

@@ -269,7 +283,7 @@ wv.data.results.dateTimeLabel = function (time) {
 return self;
 };

-wv.data.results.densify = function () {
+export function dataResultsDensify() {


What's the difference between prefixing all of the data functions with data and prefixing all of the Worldview JavaScript files with wv.? Does the extra prefix provide some value? Or are we being more verbose than we need to be?

localjo · 2018-01-08T21:32:00Z

web/js/data/cmr.js

-wv.data.cmr.client = wv.data.cmr.client || function (spec) {
+import $ from 'jquery';
+import util from '../util/util';
+import OlPolygon from 'ol/geom/polygon';


Should this be OlGeomPolygon for consistency with the other places you imported it?

Good catch!

localjo · 2018-01-08T21:34:31Z

web/js/data/ui.js

 'No results received yet. This may be due to a ' +
 'connectivity issue. Please try again later.'
 );
 };

 var updateSelection = function () {
 var $button = $('#wv-data-download-button');
- var selected = _.size(model.selectedGranules);
+ var selected = loSize(model.selectedGranules);


I think loSize is a very confusing function name to find in a context like this. Wouldn't someone's first intuition here be to think that we're selecting the granule with the lowest size from the model? I think @ZachTRice and I are in favor of spelling out the whole prefix for lodash functions; lodashSize, since lo can be kind of ambiguous.

localjo

I am seeing problems when trying to use data download in any projection other than Geographic. If I go to this link https://localhost:3000/?p=antarctic&l=VIIRS_SNPP_CorrectedReflectance_TrueColor(hidden),MODIS_Aqua_CorrectedReflectance_TrueColor(hidden),MODIS_Terra_CorrectedReflectance_TrueColor,MODIS_Terra_Brightness_Temp_Band31_Night,MODIS_Aqua_Ice_Surface_Temp_Night,MODIS_Terra_Ice_Surface_Temp_Night,MODIS_Terra_Snow_Cover,Coastlines&t=2018-01-08&z=3&v=-6881280,-3952640,6881280,3952640&ab=off&as=2018-01-01&ae=2018-01-08&av=3&al=true&download=MOD10_L2

It doesn't work, but the same link works fine on production.

localjo · 2018-01-08T21:50:29Z

web/js/data/results.js

 return granule;
 };

 return self;
 };

-wv.data.results.extentFilter = function (projection, extent) {
+export function dataResultsExtentFilter(projection, extent) {
 var self = {};


When a non-geographic projection is selected, it looks like this function gets called with extent being undefined.

localjo · 2018-01-08T21:59:52Z

web/js/map/map.js

@@ -4,6 +4,10 @@ import loIsUndefined from 'lodash/isUndefined';
 import loFind from 'lodash/find';
 import OlGeomPolygon from 'ol/geom/polygon';

+export const CRS_WGS_84 = 'EPSG:4326';


Where was this constant defined before? I suspect that maybe the bug we're seeing is related to this constant.

here withinself: https://github.com/nasa-gibs/worldview/blob/master/web/js/map/wv.map.js#L8

Benjaki2 · 2018-01-09T00:11:46Z

@localjo @ZachTRice
I believe c4d3a68 addresses the error with polar views.

localjo · 2018-01-09T14:13:04Z

Looks good! Let's merge!

Benjamin King added 7 commits January 8, 2018 14:21

make edsc methods modular

10b4249

* export as `dataHelper` instead of `L`

rename map utility methods to use universal convention

ed92e38

activate data download in main.js

265a994

add modular loaders to data download code

129035e

* all functions that were part of wv global are now exported * export reuseable constants `export const examples ='something'`

update test import to reflect export name

197f7f8

Activate data buster tests

36c260d

Fix import case

80d758b

ghost assigned Benjaki2 Jan 8, 2018

ghost added the under development label Jan 8, 2018

Benjaki2 requested review from ZachTRice and localjo January 8, 2018 20:45

Benjaki2 added ready for review and removed under development labels Jan 8, 2018

localjo changed the title ~~data-download module loaders~~ Modularize Data Download Jan 8, 2018

ZachTRice approved these changes Jan 8, 2018

View reviewed changes

localjo approved these changes Jan 8, 2018

View reviewed changes

localjo suggested changes Jan 8, 2018

View reviewed changes

localjo reviewed Jan 8, 2018

View reviewed changes

Benjamin King added 3 commits January 8, 2018 18:15

revert lodash loop to jquery

c4d3a68

Elongate lodash names

b3381cc

Fix import naming

8106bab

localjo approved these changes Jan 9, 2018

View reviewed changes

Benjaki2 merged commit 72f4104 into module-loaders Jan 9, 2018

Benjaki2 deleted the module-loaders-data-download branch January 9, 2018 15:49

ghost removed the ready for review label Jan 9, 2018

Benjaki2 mentioned this pull request Jan 9, 2018

Module loaders #503

Merged

58 tasks

localjo added this to the Module Loaders Release (v2.0) milestone Jan 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize Data Download #654

Modularize Data Download #654

Benjaki2 commented Jan 8, 2018 •

edited

Loading

ZachTRice left a comment

ZachTRice commented Jan 8, 2018

localjo left a comment

localjo Jan 8, 2018

Benjaki2 Jan 8, 2018

localjo Jan 8, 2018

Benjaki2 Jan 8, 2018

localjo Jan 8, 2018

localjo Jan 8, 2018

localjo Jan 8, 2018

localjo Jan 8, 2018

Benjaki2 Jan 8, 2018

localjo Jan 8, 2018

localjo left a comment

localjo Jan 8, 2018

localjo Jan 8, 2018

Benjaki2 Jan 8, 2018

Benjaki2 commented Jan 9, 2018 •

edited

Loading

localjo commented Jan 9, 2018

		@@ -0,0 +1,15 @@
		export function dataParser(state, errors, config) {

Modularize Data Download #654

Modularize Data Download #654

Conversation

Benjaki2 commented Jan 8, 2018 • edited Loading

ZachTRice left a comment

Choose a reason for hiding this comment

ZachTRice commented Jan 8, 2018

localjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

localjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benjaki2 commented Jan 9, 2018 • edited Loading

localjo commented Jan 9, 2018

Benjaki2 commented Jan 8, 2018 •

edited

Loading

Benjaki2 commented Jan 9, 2018 •

edited

Loading