-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor state-hashing files into state_hashing and state_storage subdirectories. #16
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
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); | ||
}; | ||
} |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 On the contrary though, I would also bet that the |
||
return mapValues(parsedQueryString, (val, key) => { | ||
const state = states.find(s => key === s.getQueryParamName()); | ||
return state ? state.translateHashToRison(val) : val; | ||
}); | ||
} |
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 |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export { | ||
default as HashingStore, | ||
} from './hashing_store'; | ||
|
||
export { | ||
default as LazyLruStore, | ||
} from './lazy_lru_store'; |
There was a problem hiding this comment.
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 thegetUnhashableStates()
call inside.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?