feat: refactor the setHash function

[EshaanAgg]

Related Issue

Fixes #807

Implementation notes

• Refactored the parts of getHashSearch to increase it's readability
• Split the setHashSearch function into two functions
  • setHashSearch: Responsible for actually setting the query parameters correctly in the URL based on the configuration options provided
  • constructQueryString: Responsible for taking in an object of params and generating a query string from them. This utility offers a layer of abstraction and can be reused in the codebase at many places

Roadblock

The setHashSeach function is responsible for creating the correct URL and setting the document.location.href to the same. This poses a problem to us, as JSDom does not mock routing and thus throws an error when we try to test the same in the unit tests for the function. It throws the error described in this Stack Overflow thread, but all of the solutions mentioned there, as when the issue for the same on the JSDom repository does not fix it. The same can be verified by pulling this PR, commenting out the test suite for setHashSearch, and then running the test suite.

I could not find the specific version of JSDom we are using in the project in the package.json, and I also had trouble understanding the testing setup in the repository. With more context, I can suggest a better approach to tackle this problem.

[PhilippWendler]

The splitting is definitively improving things already. Maybe we can refactor even further, like pushing the whole URL construction into the utility method? baseUrl and returnString could potentially even be removed as a parameter from setHashSearch, they seem to be relevant only for testing, but those tests would then test the utility method.

JSDom seems to be a transitive dependency of Jest, and package-lock.json refers to version 16.7.0. I would have expected mocking to work, though, because we successfully mock window.crypto in the same way for our tests, so mocking window.location should also be possible. What did you try in this regard?

[EshaanAgg]

The splitting is definitively improving things already. Maybe we can refactor even further, like pushing the whole URL construction into the utility method? baseUrl and returnString could potentially even be removed as a parameter from setHashSearch, they seem to be relevant only for testing, but those tests would then test the utility method.

Okay, sir. I'll refactor the URL construction into a different utility function altogether.

JSDom seems to be a transitive dependency of Jest, and package-lock.json refers to version 16.7.0. I would have expected mocking to work because we successfully mock window.crypto in the same way for our tests, so mocking window.location should also be possible. What did you try in this regard?

I tried a couple of things which were listed in the various resources I consulted online:

• Trying to delete the window/document object or reassigning the relevant properties on the same (something like the following):

delete window.location;
window.location = { assign: assignMock };

The problem with this approach that I get the error that you can't reassign or delete the properties on the read-only objects window and document.
The same happens while trying to use the defineProperty function:

const mockResponse = jest.fn();
Object.defineProperty(window, 'location', {
  value: {
    hash: {
      endsWith: mockResponse,
      includes: mockResponse,
    },
    assign: mockResponse,
  },
  writable: true,
});

• Using third-party libraries such as jest-location-mock, but no luck.
• Making use of window.location,assign rather than window.location.href, but the same throws the following error:
  

I'll try to look into the version specific resources available for the same tonight, and post updates on my research here. Thanks for the review!

[PhilippWendler]

Hm, we mock window.crypto via this line:

benchexec/benchexec/tablegenerator/react-table/src/tests/setup.js

Line 13 in 2217e9f

window.crypto = jest.fn();

That seems to work. Maybe try the same? Maybe it also only works in the setup code and not in the test itself? (If you tried out the latter.)

[EshaanAgg]

This is the error that I face with I try to mock it in the following manner. The reason for the same (as far as I understand it) is because window.crypto is not a resevered property, where as window.location or document.location is a reserved and read-only property that has been setup by JSDOM, but can't be overwritten.

[EshaanAgg]

I have updated the PR in the following manner:

• Created a utility getURLHash which accepts a URL, a object params and keepOthers boolean, and has the responsibility of generating the new URLs. This function is 100% unit testable and has been supplemented witht the all the tests.
• The function setURLHash now accepts three positional arguments (which is cleaner to read than the earlier approach of specifying the parameters in an options object) and performs the desired operation. No tests have been written for this function due to the hiccups above.
• I have also updated the codebase to make use of the new function signature everywhere.

[PhilippWendler]

Thanks! This is already a nice improvement, and I fully agree with your functionality split and testing strategy. I guess it is the best that we can do.

I have just a few minor comments left, and one question: Could you maybe have a look at the callers of setHashSearch and check whether we can simplify them? IIRC some of them call getHashSearch and merge values manually, but we could use keepOthers instead.

While writing this I noticed that at least in ReactTable.js, we have several calls that first check whether the value of a parameter changes compared to getHashSearch. We could easily simplify this if we just do nothing in setHashSearch if the URL happens to remain the same, right? I don't think this would break anything.

[EshaanAgg]

I have updated the PR to address all the review comments. Two noteworthy changes:

• To simplify the logic of the functions calling the setHashSearch method, I have removed the code where they check if the parameter already exists in the URL and instead replaced them with the setParam method, which updates the parameters as needed.
• In all the places we are calling the constructHashURL function with the boolean parameter, I have made sure we explicitly use a named variable to convert the meaning behind the boolean.

[PhilippWendler]

Nice idea to use setParam. AFAIS there are only two remaining users of setHashSearch, and in particular makeUrlFilterSerializer could be simplified by making use of the fact that setHashSearch deletes params if we pass undefined. And then I think all code uses keepOthers = true?

Maybe we can even get rid of the keepOthers parameter and setParam completely? Then we wouldn't even need options. Sorry for the back-and-forth, but sometimes refactoring is like this because you gradually end up learning more and more about the code base and seeing more possibilities to improve stuff.

[EshaanAgg]

Makes sense! It would be better to remove the setHashSearch function instead (as it has a confusing name) and keep using the setParam function in the code (with an option to pass in the history object into the same). I would also look into the refactoring the makeUrlFilterSerializer function!

[EshaanAgg]

I have made the changes as described in the comment above. I have also added a new utility method called removeParam, which does the exact opposite of the setParam method, as I felt the same was needed for code readability in the makeUrlFilterSerializer method.

PS. I am still a bit unsure if I refactored the makeUrlFilterSerializer method correctly, so would highly appreciate any review on the same. Thanks a lot @PhilippWendler sir for all the time and detailed reviews you left along the way

[PhilippWendler]

Yes, you are right, setParam is probably the better choice (or maybe setURLParameter?). I guess we could rename getHashSearch then as well, to something like getURLParameters?

[PhilippWendler]

Really nice now and a significant improvement compared to the old code. Exactly what I had hoped for in #807. Thank you very much!

[PhilippWendler]

It is good that we now document the feature of parameter deletion via undefined values, because here we can see that it was actually used already!

[PhilippWendler]

Very nice thinking!

[EshaanAgg]

Thanks a lot for the continued support and reviews!

[PhilippWendler]

For the record: We overlooked that this changed the behavior and thus broke something: The old method only either set document.location or pushed to the history, and when pushing to the history it only pushed the search string. A fix is in 3ecba1f.