Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(application): source default URL params from storage #3408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(application): source default URL params from storage #3408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 42 additions & 14 deletions packages/kuma-gui/src/app/application/components/route-view/RouteView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@
:src="uri(sources, '/me/:route', {
route: props.name,
})"
@change="resolve"
v-slot="{ data: me }"
>
<slot
v-if="me && submit"
v-if="me && submit && redirected"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the extra template guard I reused to make sure we don't show anything until we've set our default query params. This is only necessary during CI for some reason, probably due to the single tick used in the watch as a consequence of the async/await

:id="UniqueId"
name="default"
:t="t"
Expand Down Expand Up @@ -72,6 +73,7 @@ import { sources } from '@/app/me/sources'
import type { Ref } from 'vue'
import type { RouteRecordRaw, RouteLocationNormalizedLoaded } from 'vue-router'


export type RouteView = {
name: string
from: Ref
Expand All @@ -89,6 +91,11 @@ const win = window
const env = useEnv()
const can = useCan()
const uri = useUri()


let resolve: (resolved: object) => void
const meResponse = new Promise((r) => resolve = r)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Boilerplate for setting up the JS accessible promise to wait for me to resolve.


const htmlAttrs = useAttrs()
const { t } = useI18n()
const route = useRoute()
Expand Down Expand Up @@ -180,56 +187,77 @@ watch(routeParams, (val) => {
})

// when any URL params change, normalize/validate/default and reset our actual application params
const redirected = ref<boolean>(false)
watch(() => {
return Object.keys(props.params).map((item) => { return route.params[item] || route.query[item] })
}, () => {
}, async () => {
const stored = await meResponse

// merge params in order of importance/priority:
// 1. Anything stored by the user in storage
// 2. URL query params
// 3. URL path params
const params = {
...get(stored, 'params', {}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the super important bit that simplifies everything, we use the stored params first, then query, then URL params. i.e. we just continue to do as we did before. The problem we more fiddling around with Vue so we could use await here without hitting concurrency issues and making sure we always normalize route params, but only redirect once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks much cleaner now 👍

...route.query,
...route.params,
}

// normalize/validate/default all params using the RouteView :params
// 1. Ignore any `?param[]=0` type params, we just take the first one
// 2. Using normalizeUrlParam and the type information from RouteView :params convert things to the correct type i.e. null > false
// 3. Use RouteView :params to set any params that are empty, i.e. use RouteView :params as defaults.
Object.entries({
...props.params,
}).reduce((prev, [prop, def]) => {
const param = urlParam(typeof params[prop] === 'undefined' ? '' : params[prop])
prev[prop] = normalizeUrlParam(param, def)
return prev
}, routeParams as Record<string, string | number | boolean>)
}, { immediate: true })

watch(() => props.name, () => {
// we only want query params here
const params = Object.entries(routeParams || {}).reduce((prev, [key, value]) => {
if (typeof route.params[key] === 'undefined') {
prev[key] = value
// only one first load, if any params are missing from the URL/query
// redirect/add the query params to the URL.
// this ensures any JS applied defaults are reflected in the URL
if(!redirected.value) {
// we only want query params here
const params = Object.entries(routeParams || {}).reduce((prev, [key, value]) => {
if (typeof route.params[key] === 'undefined') {
prev[key] = value
}
return prev
}, {} as Record<string, string | boolean | undefined>)
if (Object.keys(params).length > 0) {
router.replace({
query: cleanQuery(params, route.query),
})
}
return prev
}, {} as Record<string, string | boolean | undefined>)
if (Object.keys(params).length > 0) {
router.replace({
query: cleanQuery(params, route.query),
})
redirected.value = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the two merged watches, you might want to look at the old file/new file outside a diff to get a better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that helps a lot and thanks for all the new comments 👍

}
}, { immediate: true })

type RouteParams = Record<string, string | boolean | number | undefined>
let newParams: RouteParams = {}

const routerPush = (params: RouteParams) => {
router.push({
name: props.name,
query: cleanQuery(params, route.query),
})
newParams = {}
}

const routeUpdate = (params: RouteParams): void => {
newParams = {
...newParams,
...params,
}
routerPush(newParams)
}

const routeReplace = (...args: RouteReplaceParams) => {
router.push(...args)
}

const routerBack = (...args: RouteReplaceParams) => {
try {
if (win.history.state.back !== null) {
Expand Down
11 changes: 11 additions & 0 deletions packages/kuma-gui/src/app/application/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type { Can } from './services/can'
import type { EnvVars } from './services/env/Env'
import Env from './services/env/Env'
import I18n from './services/i18n/I18n'
import storage from './services/storage'
import type { Source } from '@/app/application/services/data-source'
import { create, destroy, getSource, DataSourcePool } from '@/app/application/services/data-source'
import { services as kuma } from '@/app/kuma'
Expand Down Expand Up @@ -81,6 +82,9 @@ const $ = {

i18n: token<ReturnType<typeof I18n>>('i18n'),
enUs: token('i18n.locale.enUs'),

storage: token<ReturnType<typeof storage>>('application.storage'),
storagePrefix: token<string>('application.storage.prefix'),
}

const addModule = (item: RouteRecordRaw, parent?: RouteRecordRaw) => {
Expand Down Expand Up @@ -183,6 +187,13 @@ export const services = (app: Record<string, Token>): ServiceDefinition[] => {
service: () => fetch,
}],

[$.storage, {
service: storage,
arguments: [
app.storagePrefix,
],
}],

[$.can, {
service: can,
arguments: [
Expand Down
21 changes: 21 additions & 0 deletions packages/kuma-gui/src/app/application/services/storage/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export default (prefix: string = 'kumahq-app', storage: Storage = window.localStorage) => {
const get = async (key: string): Promise<object> => {
try {
return JSON.parse(storage.getItem(`${prefix}:${key}`) ?? '{}')
} catch (e) {
console.error(e)
}
return {}
}
const set = async (key: string, value: object): Promise<object> => {
try {
storage.setItem(`${prefix}:${key}`, JSON.stringify(value))
return value
} catch (e) {
console.error(e)
}
return {}
}

return { get, set }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to get rid of console.errors entirely (and throw/catch errors or something else), we'd have to decide what to do here. Thinking about the type of error we'd get here, I'd be almost 100% on just lint-ignoring these ones.

3 changes: 1 addition & 2 deletions packages/kuma-gui/src/app/me/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,13 @@ type Sources = ReturnType<typeof sources>

const $ = {
sources: token<Sources>('me.sources'),
storagePrefix: token<string>('me.storage.prefix'),
}
export const services = (app: Record<string, Token>): ServiceDefinition[] => {
return [
[$.sources, {
service: sources,
arguments: [
$.storagePrefix,
app.storage,
],
labels: [
app.sources,
Expand Down
44 changes: 19 additions & 25 deletions packages/kuma-gui/src/app/me/sources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,32 @@ import merge from 'deepmerge'

import { defineSources } from '../application/services/data-source'

export const sources = (prefix: string = 'me', storage: Storage = window.localStorage) => {
const get = async (key: string): Promise<object> => {
try {
return JSON.parse(storage.getItem(`${prefix}:${key}`) ?? '{}')
} catch (e) {
console.error(e)
}
return {}
}
const set = async (key: string, value: object): Promise<object> => {
try {
storage.setItem(`${prefix}:${key}`, JSON.stringify(value))
return value
} catch (e) {
console.error(e)
}
return {}
}
type Storage = {
get: (key: string) => Promise<object>
set: (key: string, value: object) => Promise<object>
}
export const sources = ({ get, set }: Storage) => {
return defineSources({
// retrieves both route and global app prefs and merges them
// anything route specific overwriting anything global/app specific
'/me/:route': async (params) => {
const json = await get(params.route)
const res = merge({
params: {
size: 50,
const [app, route] = await Promise.all([
get('/'),
get(params.route),
])
return merge(
{
params: {
size: 50,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a plan to use this as the "centralized 50". We currently set size=50 in every single list view, I'd like to remove those and make it optional to specific an exact hardcoded value (you'd still have to specify size as a route param to get the type goodness etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably move these things to the global me as defaults 🤔

},
}, json)
return res
app,
route,
)
},
'/me/:route/:data': async (params) => {
const json = JSON.parse(params.data)
const res = merge<object>(await get(params.route), json)

set(params.route, res)
},
})
Expand Down
Loading