URLWrapper: mediate all read/write interaction between app and url #2726

alisman · 2019-09-20T21:53:08Z

The URL wrapper allows app to read and write from the browser URL in a controlled way. Each page has an instance of URLWrapper which defines the parameters used by that page. These params are always observable on the URLWrapper (even when they aren't present in a particular URL).

The URLWrapper also abstracts the loading and creation of "sessions." Sessions are created when URL parameters would create a URL that is too long for the browser. We then store the parameters in a remote session. When the user changes a "session" parameter of the URL, a new session is automatically created. The URLWrapper also loads session data. It allows developers to work without paying mind whether URL is stored in session or in browser url.

alisman · 2019-10-04T15:50:50Z

src/pages/resultsView/ResultsViewPage.tsx

+
+    const resultsViewPageStore = new ResultsViewPageStore(appStore, getBrowserWindow().globalStores.routing, urlWrapper);
+
+    // whenever study list changes, reinit driver annotation settings


@adamabeshouse i leave this for reference in this review because it's important to make sure we are accomplishing all this in new model

alisman · 2019-10-04T15:54:05Z

src/shared/components/query/QueryStore.ts

-            params.genetic_profile_ids_PROFILE_MRNA_EXPRESSION,
-            params.genetic_profile_ids_PROFILE_METHYLATION,
-            params.genetic_profile_ids_PROFILE_PROTEIN_EXPRESSION,
-            params.genetic_profile_ids_PROFILE_GENESET_SCOR


i must have reformatted this entire file.

alisman · 2019-10-04T15:59:53Z

src/shared/lib/URLWrapper.ts


    constructor(
        protected routing:ExtendedRouterStore,
        protected properties:Property<QueryParamsType>[]
    ) {
        const initValues:Partial<QueryParamsType> = {};
        for (const property of properties) {
-            initValues[property.name] = undefined;
+            initValues[property.name] = (routing.query as QueryParamsType)[property.name];


as a reminder, we realized we could do this up front instead of starting at undefined

alisman · 2019-10-04T16:03:26Z

src/pages/resultsView/ResultsViewPage.tsx

@@ -466,7 +476,7 @@ export default class ResultsViewPage extends React.Component<IResultsViewPagePro
                        {
                            // we don't show the result tabs if we don't have valid query
                            (this.showTabs && !this.resultsViewPageStore.genesInvalid && !this.resultsViewPageStore.isQueryInvalid) && (
-                                <MSKTabs key={this.resultsViewPageStore.rvQuery.hash} activeTabId={this.currentTab(this.resultsViewPageStore.tabId)} unmountOnHide={false}
+                                <MSKTabs activeTabId={this.currentTab(this.resultsViewPageStore.tabId)} unmountOnHide={false}


@adam I took the key off of this. We need to discuss why we had key in the first place. Obviously to force re-mounting. But why? There was a reason!

man I wish we'd left a comment. clearly there was a reason because we went to the trouble of creating that hash.

adamabeshouse · 2019-10-04T20:14:29Z

src/shared/components/oncoprint/ResultsViewOncoprint.tsx

@@ -1072,6 +1071,7 @@ export default class ResultsViewOncoprint extends React.Component<IResultsViewOn
    }

    public render() {
+        console.log("Oncoprint rendering");


src/pages/resultsView/ResultsViewPage.tsx

src/pages/resultsView/ResultsViewURLWrapper.ts

adamabeshouse · 2019-10-04T20:37:53Z

src/shared/lib/URLWrapper.spec.ts

+import ExtendedRouterStore from "shared/lib/ExtendedRouterStore";
+import sinon from "sinon";
+
+describe("URLWrapper", () => {


this is really ResultsViewURLWRapper.spec.ts

I think it might be ok to test a superclass using it's subclass

adamabeshouse · 2019-10-04T20:38:56Z

src/shared/lib/URLWrapper.ts

-                this.query[property.name] = query[property.name];
+                // @ts-ignore
+                this.setProperty(property, query);
+                //this.query[property.name] = typeof query[property.name] === "string" ? decodeURIComponent(query[property.name]) : undefined;


commented line

src/shared/lib/URLWrapper.ts

adamabeshouse · 2019-10-04T20:40:37Z

src/shared/lib/URLWrapper.ts

@@ -39,6 +52,23 @@ export default class URLWrapper<QueryParamsType> {
        this.routing.updateRoute(query as any);
    }

+    private setProperty(property:Property<QueryParamsType>, query:QueryParamsType){


lets call this syncProperty for more semantic readability?

adamabeshouse · 2019-10-04T20:41:24Z

src/shared/lib/URLWrapper.ts

+    }
+
+    private _setPropertyAndHandleUndefined(property:Property<QueryParamsType>, value:string|undefined){
+        // @ts-ignore


not huge deal, but how about trySyncProperty? a little more readable to me

adamabeshouse · 2019-10-04T20:43:31Z

src/shared/lib/URLWrapper.ts

+        // if it's still undefined, then check aliases
+        if (this.query[property.name] === undefined && property.aliases && property.aliases.length) {
+            for (const alias of property.aliases) {
+                this._setPropertyAndHandleUndefined(property, query[alias]);


I think it would be most idiomatic to have the helper return boolean depending whether it successfully set a value. then you can do if (this.trySyncProperty(..)) break;

a little cleaner I think

but not completely necessary

…fix bugs when they change dynamically Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

… by not rendering unless everything is ready Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

…on refresh Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

…oncoprint Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

…h browser back/forward

…etc)

Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

@computed

…oblems probably due to breaking @computed's connection to the reaction Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

…pendencies are complete Signed-off-by: Abeshouse, Adam A./Sloan Kettering Institute <[email protected]>

alisman added DO NOT MERGE do not review labels Sep 20, 2019

alisman force-pushed the urlWrapper branch from 051170d to 73b7c36 Compare October 3, 2019 21:29

alisman removed DO NOT MERGE do not review labels Oct 4, 2019

alisman force-pushed the urlWrapper branch from 73b7c36 to 2d8d506 Compare October 4, 2019 14:43

alisman commented Oct 4, 2019

View reviewed changes

alisman force-pushed the urlWrapper branch from c7c1505 to 0c6aeeb Compare October 4, 2019 18:03