Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Refactor state-hashing files into state_hashing and state_storage subdirectories. #16

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/ui/public/chrome/directives/kbn_chrome.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import $ from 'jquery';

import './kbn_chrome.less';
import UiModules from 'ui/modules';
import { UnhashStatesProvider } from 'ui/state_management/unhash_states';
import {
getUnhashableStatesProvider,
unhashUrl,
} from 'ui/state_management/state_hashing';

export default function (chrome, internals) {

Expand All @@ -28,15 +31,16 @@ export default function (chrome, internals) {

controllerAs: 'chrome',
controller($scope, $rootScope, $location, $http, Private) {
const unhashStates = Private(UnhashStatesProvider);
const getUnhashableStates = Private(getUnhashableStatesProvider);

// are we showing the embedded version of the chrome?
internals.setVisibleDefault(!$location.search().embed);

// listen for route changes, propogate to tabs
const onRouteChange = function () {
let { href } = window.location;
internals.trackPossibleSubUrl(unhashStates.inAbsUrl(href));
const urlWithHashes = window.location.href;
const urlWithStates = unhashUrl(urlWithHashes, getUnhashableStates());
internals.trackPossibleSubUrl(urlWithStates);
};

$rootScope.$on('$routeChangeSuccess', onRouteChange);
Expand Down
14 changes: 10 additions & 4 deletions src/ui/public/share/directives/share_object_url.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import '../styles/index.less';
import LibUrlShortenerProvider from '../lib/url_shortener';
import uiModules from 'ui/modules';
import shareObjectUrlTemplate from 'ui/share/views/share_object_url.html';
import { UnhashStatesProvider } from 'ui/state_management/unhash_states';
import {
getUnhashableStatesProvider,
unhashUrl,
} from 'ui/state_management/state_hashing';
import { memoize } from 'lodash';

app.directive('shareObjectUrl', function (Private, Notifier) {
Expand Down Expand Up @@ -71,12 +74,15 @@ app.directive('shareObjectUrl', function (Private, Notifier) {
};

// since getUrl() is called within a watcher we cache the unhashing step
const unhashStatesInAbsUrl = memoize((absUrl) => {
return Private(UnhashStatesProvider).inAbsUrl(absUrl);
const unhashAndCacheUrl = memoize((urlWithHashes, unhashableStates) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memoize only caches based on the first argument (and should really throw when more than one is passed) so we should probably move the getUnhashableStates() call inside.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That or we could stop using memoize I suppose

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Let's take it out and check for a performance degradation due to the watcher?

const urlWithStates = unhashUrl(urlWithHashes, unhashableStates);
return urlWithStates;
});

$scope.getUrl = function () {
let url = unhashStatesInAbsUrl($location.absUrl());
const urlWithHashes = $location.absUrl();
const getUnhashableStates = Private(getUnhashableStatesProvider);
let url = unhashAndCacheUrl(urlWithHashes, getUnhashableStates());

if ($scope.shareAsEmbed) {
url = url.replace('?', '?embed=true&');
Expand Down
9 changes: 4 additions & 5 deletions src/ui/public/state_management/__tests__/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import { encode as encodeRison } from 'rison-node';
import 'ui/private';
import Notifier from 'ui/notify/notifier';
import StateManagementStateProvider from 'ui/state_management/state';
import { UnhashStatesProvider } from 'ui/state_management/unhash_states';
import { HashingStore } from 'ui/state_management/hashing_store';
import { unhashQueryString } from 'ui/state_management/state_hashing';
import { HashingStore } from 'ui/state_management/state_storage';
import StubBrowserStorage from 'test_utils/stub_browser_storage';
import EventsProvider from 'ui/events';

Expand All @@ -35,9 +35,8 @@ describe('State Management', function () {
const hashingStore = new HashingStore({ store });
const state = new State(param, initial, { hashingStore, notify });

const getUnhashedSearch = (state) => {
const unhashStates = Private(UnhashStatesProvider);
return unhashStates.inParsedQueryString($location.search(), [ state ]);
const getUnhashedSearch = state => {
return unhashQueryString($location.search(), [ state ]);
};

return { notify, store, hashingStore, state, getUnhashedSearch };
Expand Down
87 changes: 0 additions & 87 deletions src/ui/public/state_management/__tests__/unhash_states.js

This file was deleted.

6 changes: 4 additions & 2 deletions src/ui/public/state_management/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import EventsProvider from 'ui/events';
import Notifier from 'ui/notify/notifier';
import KbnUrlProvider from 'ui/url';

import { HashingStore } from './hashing_store';
import { LazyLruStore } from './lazy_lru_store';
import {
HashingStore,
LazyLruStore,
} from './state_storage';

const MAX_BROWSER_HISTORY = 50;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import expect from 'expect.js';
import ngMock from 'ng_mock';
import sinon from 'auto-release-sinon';

import StateProvider from 'ui/state_management/state';
import { unhashUrl } from 'ui/state_management/state_hashing';

describe('unhashUrl', () => {
let unhashableStates;

beforeEach(ngMock.module('kibana'));

beforeEach(ngMock.inject(Private => {
const State = Private(StateProvider);
const unhashableState = new State('testParam');
sinon.stub(unhashableState, 'translateHashToRison').withArgs('hash').returns('replacement');
unhashableStates = [unhashableState];
}));

describe('does nothing', () => {
it('if missing input', () => {
expect(() => {
unhashUrl();
}).to.not.throwError();
});

it('if just a host and port', () => {
const url = 'https://localhost:5601';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if just a path', () => {
const url = 'https://localhost:5601/app/kibana';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if just a path and query', () => {
const url = 'https://localhost:5601/app/kibana?foo=bar';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if empty hash with query', () => {
const url = 'https://localhost:5601/app/kibana?foo=bar#';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if empty hash without query', () => {
const url = 'https://localhost:5601/app/kibana#';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if empty hash without query', () => {
const url = 'https://localhost:5601/app/kibana#';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if hash is just a path', () => {
const url = 'https://localhost:5601/app/kibana#/discover';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});

it('if hash does not have matching query string vals', () => {
const url = 'https://localhost:5601/app/kibana#/discover?foo=bar';
expect(unhashUrl(url, unhashableStates)).to.be(url);
});
});

it('replaces query string vals in hash for matching states with output of state.toRISON()', () => {
const urlWithHashes = 'https://localhost:5601/#/?foo=bar&testParam=hash';
const exp = 'https://localhost:5601/#/?foo=bar&testParam=replacement';
expect(unhashUrl(urlWithHashes, unhashableStates)).to.be(exp);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default function getUnhashableStatesProvider(getAppState, globalState) {
return function getUnhashableStates() {
return [getAppState(), globalState].filter(Boolean);
};
}
11 changes: 11 additions & 0 deletions src/ui/public/state_management/state_hashing/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export {
default as getUnhashableStatesProvider,
} from './get_unhashable_states_provider';

export {
default as unhashQueryString,
} from './unhash_query_string';

export {
default as unhashUrl,
} from './unhash_url';
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { mapValues } from 'lodash';

export default function unhashQueryString(parsedQueryString, states) {
Copy link
Owner

@spalger spalger Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you opposed to exporting the named function? It's a pattern I've been following for some time and I really like how it encourages using the same name for a value everywhere, and choosing names that make sense in multiple contexts.

Copy link
Author

@cjcenizal cjcenizal Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally see your point. We can write the code in a certain way that encourages developers to use the "right name" when importing the module.

On the other hand, I think the pattern of using default exports to indicate a single export from a module is also pretty common, especially for single-function modules like these. I think a practice like "alias the default export with same name as the file" is a pretty straightforward practice to adopt, and will address the problem of developers naming the import poorly.

The AirBnB style guide recommends preferring a default export with modules with only one export, though they don't do a very good job of explaining why.

I don't think we should adhere to it for the sake of dogma, but it's worth some thought as to why they recommend this practice. Bullet 3.2 of this guide has a pretty good possible explanation. MDN states: "This [default export] value is to be considered as the "main" exported value since it will be the simplest to import."

Copy link
Owner

@spalger spalger Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've seen that rule enforced in their eslint config as well. Like you said, the rule is based on the number of exports, which makes little to no sense to me. Importing code needs to know how many values you export? What if you decide to add a second import? Do you have to export a single value multiple ways now?

I think a lot of this stuff is so new that people are simply picking rules so they have a standard to follow. I would bet that this rule was chosen because module.exports = and export default are very similar and module.exports = is the most common export style in CommonJS. This means that npm modules and app code are imported the same way, which I'm fine with.

On the contrary though, I would also bet that the module.exports = pattern beat out exports.name = because we didn't have dereferencing assignment. Now that we do, I almost always import the specific named methods of a module when I'm pulling it in (unless that module exports a single function).

return mapValues(parsedQueryString, (val, key) => {
const state = states.find(s => key === s.getQueryParamName());
return state ? state.translateHashToRison(val) : val;
});
}
36 changes: 36 additions & 0 deletions src/ui/public/state_management/state_hashing/unhash_url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import {
parse as parseUrl,
format as formatUrl,
} from 'url';

import unhashQueryString from './unhash_query_string';

export default function unhashUrl(urlWithHashes, states) {
if (!urlWithHashes) return urlWithHashes;

const urlWithHashesParsed = parseUrl(urlWithHashes, true);
if (!urlWithHashesParsed.hostname) {
// passing a url like "localhost:5601" or "/app/kibana" should be prevented
throw new TypeError(
'Only absolute urls should be passed to `unhashUrl()`. ' +
'Unable to detect url hostname.'
);
}

if (!urlWithHashesParsed.hash) return urlWithHashes;

const appUrl = urlWithHashesParsed.hash.slice(1); // trim the #
if (!appUrl) return urlWithHashes;

const appUrlParsed = parseUrl(urlWithHashesParsed.hash.slice(1), true);
if (!appUrlParsed.query) return urlWithHashes;

const appQueryWithoutHashes = unhashQueryString(appUrlParsed.query || {}, states);
return formatUrl({
...urlWithHashesParsed,
hash: formatUrl({
pathname: appUrlParsed.pathname,
query: appQueryWithoutHashes,
})
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import sinon from 'sinon';
import { encode as encodeRison } from 'rison-node';

import StubBrowserStorage from 'test_utils/stub_browser_storage';
import { HashingStore } from 'ui/state_management/hashing_store';
import { HashingStore } from 'ui/state_management/state_storage';

const setup = ({ createHash } = {}) => {
const store = new StubBrowserStorage();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import sinon from 'sinon';
import { times, sum, padLeft } from 'lodash';

import StubBrowserStorage from 'test_utils/stub_browser_storage';
import { LazyLruStore } from '../lazy_lru_store';
import { LazyLruStore } from '..';

const setup = (opts = {}) => {
const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const TAG = 'h@';
* hash. This hash is then returned so that the item can be received
* at a later time.
*/
export class HashingStore {
export default class HashingStore {
constructor({ store, createHash, maxItems } = {}) {
this._store = store || window.sessionStorage;
if (createHash) this._createHash = createHash;
Expand Down
7 changes: 7 additions & 0 deletions src/ui/public/state_management/state_storage/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export {
default as HashingStore,
} from './hashing_store';

export {
default as LazyLruStore,
} from './lazy_lru_store';
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const DEFAULT_IDEAL_CLEAR_RATIO = 100;
*/
const DEFAULT_MAX_IDEAL_CLEAR_PERCENT = 0.3;

export class LazyLruStore {
export default class LazyLruStore {
constructor(opts = {}) {
const {
id,
Expand Down
Loading