Preserve query parameters when rebasing location path #67

braxnu · 2018-03-01T16:02:55Z

Description

The fix prevents the library from discarding the query parameters from URL when opening a Zambezi resource.

Example:

// Current behaviour: URL are replaced as follows:
https://example.com/app/Monitor?debug=true => https://example.com/app/Monitor
https://example.com/app?debug=true#/Monitor => https://example.com/app/Monitor

// Expected behaviour is:
https://example.com/app/Monitor?debug=true => https://example.com/app/Monitor?debug=true
https://example.com/app?debug=true#/Monitor => https://example.com/app/Monitor?debug=true

Motivation and Context

Preserving the parameters in the current window, so they are effective after a window load and reload.
Passing the parameters to child windows so the can be started with the same parameters (e.g. debug=true)

How Was This Tested?

Case 1

Query parameters string is added to the current URL
location's module pushState method is called with a new path
Assertion is made to ensure that query parameters are preserved

Case 2

Query parameters string and hash path are added to the current URL
location module is forcibly re-instantiated
Assertion is made to ensure that query parameters are preserved

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change follows the style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the contribution guidelines
I have added tests to cover my changes
All new and existing tests passed

@mstade @matthewdunsdon @Poetro @FabienDeshayes

mstade

Thank you for the PR!

These changes are quite difficult to follow, and the issue at hand is lost on me. Can you please update the PR description to match this template please, and provide as much detail as possible (without giving away privileged information):

https://github.com/zambezi/address/blob/master/.github/PULL_REQUEST_TEMPLATE.md

Finally, I'm not convinced these changes require changing the location module from a singleton to an instantiated module with local state as opposed to global state. The reasons for this is that location is inherently global (i.e. window) state, and while a locally instantiated module may make testing easier in the short run, it also introduces significant risk of breaking this library in unexpected ways that may not be covered by the current test suite.

I'd like to see this revert to a more limited set of changes that cover the issue at hand, and that issue clearly described so I can better appreciate it. There's a lot of context lost on me I'm afraid. If you could improve on this I'm sure we can make quickly make some headway. Many thanks!

mstade · 2018-03-12T15:05:06Z

bundle.js

@@ -6,7 +6,7 @@ define(function(require) {
  , httpStatusCode: require('./http-status-code')
  , interpolate: require('./interpolate')
  , into: require('./into')
-  , location: require('./location')
+  , location: require('./location')()


The location module is effectively a singleton prior to these changes. I can understand how this makes testing more difficult, however it seems to me that by changing this to be an instantiated module with local state, this also breaks the API. Anything now depending on address.location can no longer rely on the state of this module to be the same as what's used by address internally. At least, that's how I read this – it's a bit difficult to follow the changes.

braxnu · 2018-03-12T20:12:35Z

Thanks a lot for your comment @mstade

I tried my best to keep the changes to a minimum to achieve the desired result. The diff may not look clear at first sight as Github doesn't ignore whitespace change. To make it easier I'm pasting the whitespac-ignored diff of location.js below.

diff --git a/location.js b/location.js
index 2bc4e9d..4a1c317 100644
--- a/location.js
+++ b/location.js
@@ -1,4 +1,15 @@
 define(function(require) {
+  var instance
+
+  return function(force) {
+    if (!instance || force) {
+      instance = getInstance()
+    }
+
+    return instance
+  }
+
+  function getInstance() {
     var findClosest = require('./find-closest')
       , rebind = require('./rebind')
       , dispatcher = require('d3-dispatch').dispatch('statechange')
@@ -12,7 +23,7 @@ define(function(require) {

     if (isHashPath(location.hash)) {
       // Redirect current hash fragment location to "real" path
-    history.replaceState(null, null, rebase(fullPath(location)))
+      history.replaceState(null, null, rebase(fullPath(location)) + location.search)
     }

     var api =
@@ -51,7 +62,7 @@ define(function(require) {
       if (path === getState()) {
         return false
       } else {
-      method({ base: base, path: path }, null, rebase(path))
+        method({ base: base, path: path }, null, rebase(path) + location.search)
         return path
       }
     }
@@ -194,4 +205,5 @@ define(function(require) {
     function trimSlashes(path) {
       return (path || '').replace(/^\/+|\/+$/g, '')
     }
+  }
 })

I've also added a templated PR description as you requested.

In the latest commits I made sure the location module is a singleton as it was before, so we're now guaranteed, that only one instance of the module exists (as before changes). The only thing that changed is the moment the module is instantiated (this is to accommodate for tests).

Just to make it clear, window.location object is always the global one regardless of the changes. I'm not entirely sure what you mean by local/global state. What changed is the scope depth at which the module code is executed, but that also doesn't matter as the returned API is exactly the same as before.

I'm not sure I can keep the changes volume to less that they are now to achieve the same result and provide the test coverage at the same time. The only way I could do it is refactoring the module (and the dependant code) on a broader scale, but I guess we want to avoid that as much as possible.

Your concern about braking the location module API is totally fair. Luckily this module is bundled by webpack and as far as I was able to check it is never required by filename from 3rd party code (only the singleton instance is exposed from bundle).

I hope this makes it easier to review :)

mstade

This is a lot better, thank you so much @braxnu! Let me address (no pun intended) some of your comments:

Just to make it clear, window.location object is always the global one regardless of the changes. I'm not entirely sure what you mean by local/global state. What changed is the scope depth at which the module code is executed, but that also doesn't matter as the returned API is exactly the same as before.

Yes, window.location is always global and this was my point – location is also global due to the fact that it's state is effectively stored by window.global, there's no getting away from this without massive refactoring just as you say. This is why I was a bit worried about going away from its singleton nature, since you can (and indeed, some consumers have in the past) get to the location module outside of the address bundle. You've solved this neatly by making constructor function more or less memoized, this should do fine.

This is a good PR, but I'm not entirely sure it should work the way you describe, specifically around query parameters preceding a fragment based path:

// Current behaviour: URL are replaced as follows:
https://example.com/app/Monitor?debug=true => https://example.com/app/Monitor
https://example.com/app?debug=true#/Monitor => https://example.com/app/Monitor

// Expected behaviour is:
https://example.com/app/Monitor?debug=true => https://example.com/app/Monitor?debug=true
https://example.com/app?debug=true#/Monitor => https://example.com/app/Monitor?debug=true

I agree with the first case, the debug parameter should not be lost. But in the second case, where there is a fragment based path, I'm not convinced the debug parameter should be kept. I expect the fragment based path to trump the real path entirely, query parameters and all. Consider the following case:

https://example.com/app?debug=true#/Monitor?wibble=bob

What should the end result be? In this case, merging might seem like a decent solution, so something like this:

https://example.com/app?debug=true#/Monitor?wibble=bob => https://example.com/app/Monitor?debug=true&wibble=bob

However, what about this case:

https://example.com/app?debug=true#/Monitor?debug=false

Should the value of debug be true, false, or maybe even true,false? I don't know. Intuitively I expect the hash fragment path to just win, it's the simplest and least astonishing mechanic to me, but maybe I'm being too simplistic?

mstade · 2018-03-13T14:29:25Z

test/location.test.js

+
+          window.history.replaceState(
+            null, null,
+            loc.origin + loc.pathname + '?' + originalQuery.toString()


Instead of all this, can we just have `/?foo=bar&fizz=buzz' or something? It's a bit hard to follow this test.

mstade · 2018-03-13T14:32:00Z

test/location.test.js

+          var newQuery = new URLSearchParams(window.location.search.substring(1))
+
+          expect(newQuery.get('foo')).to.equal('bar')
+          expect(newQuery.get('fizz')).to.equal('buzz')


Same here, couldn't we just expect(location.getState()).to.equal('/base/foo?foo=bar&fizz=buzz')? By the way, should location.pushState really preserve the query parameters like this? I'd expect any query parameters to be lost once a new state is pushed, unless those are indeed specified in the new state.

mstade · 2018-03-13T14:33:17Z

test/location.test.js

+            null, null,
+            loc.origin + loc.pathname +
+            '?' + originalQuery.toString() +
+            '#/app/workspaces'


I'd prefer it if we could use explicit constants wherever possible to make these tests more readable. So instead of all this string building, just /?foo=bar&fizz=buzz#/app/workspaces or whatever is relevant test data.

mstade · 2018-03-13T14:34:35Z

test/location.test.js

+          var newQuery = new URLSearchParams(loc.search.substring(1))
+
+          expect(newQuery.get('foo')).to.equal('bar')
+          expect(newQuery.get('fizz')).to.equal('buzz')


As I mentioned in my general review, I'm not so sure this case actually makes sense. Any query parameters present in the fragment based path should remain, for sure, but anything in the real path should probably be discarded. Let's discuss that in the more general thread.

also: - update dependencies - fix test setup - fix failing tests

Poetro · 2018-06-14T12:23:56Z

location.js

+      , dispatcher = require('d3-dispatch').dispatch('statechange')
+      , history = window.history
+      , location = window.location
+      , on = require('./on')


Could we move the requires outside of the function call? I'm not comfortable to have constants re-imported here. Of course we should keep the no-require stuff inside.

Poetro · 2018-06-14T12:24:55Z

test/address.test.js

@@ -223,7 +223,7 @@ define(function(require) {
            , timeout: 30
            }

-        expect(zapp.rootResource()).to.be.undefined
+        // expect(zapp.rootResource()).to.be.undefined


Why did the API change in this?

braxnu added 3 commits March 1, 2018 15:47

Preserve query parameters when rebasing location path

cf6242d

convert location module into factory to allow testing

0470195

test query params preserving when "upgrading" from hash path

f65a135

mstade requested changes Mar 12, 2018

View reviewed changes

braxnu added 3 commits March 12, 2018 18:10

make sure there is only one instance of location module

324cef0

revert the changes to be minimally intrusive

1456402

allow force reinstantiation for testing

5ca13b2

mstade reviewed Mar 13, 2018

View reviewed changes

mstade requested changes Mar 13, 2018

View reviewed changes

preserve consolidated query params only during module load

540b06e

also: - update dependencies - fix test setup - fix failing tests

Poetro reviewed Jun 14, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve query parameters when rebasing location path #67

Preserve query parameters when rebasing location path #67

braxnu commented Mar 1, 2018 •

edited

Loading

mstade left a comment •

edited

Loading

mstade Mar 12, 2018

braxnu commented Mar 12, 2018 •

edited

Loading

mstade left a comment

mstade Mar 13, 2018

mstade Mar 13, 2018

mstade Mar 13, 2018

mstade Mar 13, 2018

Poetro Jun 14, 2018

Poetro Jun 14, 2018

Preserve query parameters when rebasing location path #67

Are you sure you want to change the base?

Preserve query parameters when rebasing location path #67

Conversation

braxnu commented Mar 1, 2018 • edited Loading

Description

Motivation and Context

How Was This Tested?

Case 1

Case 2

Types of changes

Checklist:

mstade left a comment • edited Loading

Choose a reason for hiding this comment

mstade Mar 12, 2018

Choose a reason for hiding this comment

braxnu commented Mar 12, 2018 • edited Loading

mstade left a comment

Choose a reason for hiding this comment

mstade Mar 13, 2018

Choose a reason for hiding this comment

mstade Mar 13, 2018

Choose a reason for hiding this comment

mstade Mar 13, 2018

Choose a reason for hiding this comment

mstade Mar 13, 2018

Choose a reason for hiding this comment

Poetro Jun 14, 2018

Choose a reason for hiding this comment

Poetro Jun 14, 2018

Choose a reason for hiding this comment

braxnu commented Mar 1, 2018 •

edited

Loading

mstade left a comment •

edited

Loading

braxnu commented Mar 12, 2018 •

edited

Loading