feat(history-service): support multiple location changes in one navigation #496
Conversation
stemey
commented
May 20, 2019
stemey
commented
- add RootHistoryService
There was a problem hiding this comment.
I only reviewed createRootLocationTransformer including tests. I noticed that TSLint and Prettier were not used and the CI/CD did not pass. Please revise your code so that it meets all formal quality criteria. After that I will continue with the content review. In general, this PR should also include adjustments to the History Service documentation on FeatureHub.io: https://feature-hub.io/docs/guides/sharing-the-browser-history
Despite all my comments, thank you for your contribution to this project!
packages/history-service/src/__tests__/create-root-location-transformer.test.ts
Outdated
Show resolved
Hide resolved
packages/history-service/src/__tests__/create-root-location-transformer.test.ts
Outdated
Show resolved
Hide resolved
packages/history-service/src/__tests__/create-root-location-transformer.test.ts
Outdated
Show resolved
Hide resolved
packages/history-service/src/__tests__/create-root-location-transformer.test.ts
Outdated
Show resolved
Hide resolved
packages/history-service/src/__tests__/create-root-location-transformer.test.ts
Outdated
Show resolved
Hide resolved
| ...consumerLocations: ConsumerLocation[] | ||
| ): history.Location { | ||
| if (options.primaryConsumerUid && !consumerLocations.some((consumerLocation) => consumerLocation.historyKey===options.primaryConsumerUid)) { | ||
| throw new Error('primary consumer is mandatory') |
There was a problem hiding this comment.
When writing the error messages, please follow the style that prevails in this project. The first letter is capitalized and we have punctuation.
There was a problem hiding this comment.
And please add a test for this error, we expect your code to have 100% test coverage.
| consumerLocation.location, | ||
| rootlocation, | ||
| consumerLocation.historyKey | ||
| ) as history.Location; |
There was a problem hiding this comment.
This cast is not correct and obscures potential errors. Shouldn't the return value of the method rather be "LocationDescriptorObject"?
There was a problem hiding this comment.
yes, we should change ConsumerLocation type. It should contain LocationDescriptorObject. We don't even support hash for any consumer but the primary. I am not sure if we should handle parameters and paths - path is usually enough and could be encoded much easier
There was a problem hiding this comment.
And what about the key in Location? We cannot properly multiplex that, I guess. It probably doesn't have a meaning for a consumer history. All appearances of Location in the consumerHistory should be cleaned up and a custom Location type without key should be used. I think the key is currently ignored completely.
| @@ -22,6 +27,10 @@ export interface RootLocationTransformer { | |||
| currentRootLocation: history.Location, | |||
| consumerUid: string | |||
| ): history.LocationDescriptorObject; | |||
|
|
|||
| createLocation( | |||
There was a problem hiding this comment.
We should find a better name for this method. How about createRootLocationForMultipleConsumers?
stemey
force-pushed
the
feat/multiple-location-transformer
branch
from
dfc5628
to
cad9730
Compare
stemey
force-pushed
the
feat/multiple-location-transformer
branch
2 times, most recently
from
7a56de9
to
803ff7f
Compare
stemey
force-pushed
the
feat/multiple-location-transformer
branch
4 times, most recently
from
65d7ac6
to
ca40207
Compare
…ation - add RootHistoryService fix(history-service): new API fix(history-service): remove idea files fix(history-service): fix linter errors and add tests fix(history-service): isssues
stemey
force-pushed
the
feat/multiple-location-transformer
branch
from
ca40207
to
e75e253
Compare
