Skip to content

Commit

Permalink
refactor(router): replace shouldSkip with componentConfig (dai-shi#834)
Browse files Browse the repository at this point in the history 
`shouldSkip` was trying to be more flexible, but it's hardly
understandable.
Replacing with `componentConfig` for now and let's see how it could be
in the future.
  • Loading branch information
@dai-shi
dai-shi authored Aug 17, 2024
1 parent 85a1171 commit d5b19bb
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 166 deletions.
119 changes: 35 additions & 84 deletions packages/waku/src/router/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
import type {
ComponentProps,
FunctionComponent,
MutableRefObject,
ReactNode,
AnchorHTMLAttributes,
ReactElement,
Expand All @@ -27,10 +26,10 @@ import { prefetchRSC, Root, Slot, useRefetch } from '../client.js';
import {
getComponentIds,
getInputString,
SHOULD_SKIP_ID,
COMPONENT_CONFIGS_ID,
LOCATION_ID,
} from './common.js';
import type { RouteProps, ShouldSkip } from './common.js';
import type { RouteProps, ComponentConfigs } from './common.js';

declare global {
interface ImportMeta {
Expand Down Expand Up @@ -62,7 +61,7 @@ const parseRoute = (url: URL): RouteProps => {
type ChangeRoute = (
route: RouteProps,
options?: {
checkCache?: boolean;
unstable_checkCache?: boolean; // TODO this doesn't work 100% correctly
skipRefetch?: boolean;
},
) => void;
Expand Down Expand Up @@ -235,59 +234,36 @@ export function Link({
}

const getSkipList = (
shouldSkip: ShouldSkip | undefined,
componentConfigs: ComponentConfigs | undefined,
componentIds: readonly string[],
route: RouteProps,
cached: Record<string, RouteProps>,
): string[] => {
const shouldSkipObj = Object.fromEntries(shouldSkip || []);
return componentIds.filter((id) => {
const prevProps = cached[id];
if (!prevProps) {
return false;
}
const shouldCheck = shouldSkipObj[id];
if (!shouldCheck) {
return false;
}
if (shouldCheck[0] && route.path !== prevProps.path) {
return false;
const config = componentConfigs?.[id];
if (!config) {
return false; // no config
}
if (shouldCheck[1] && route.query !== prevProps.query) {
return false;
const [render] = config;
if (!render) {
return true; // no component
}
return true;
return render === 'static'; // static component
});
};

const equalRouteProps = (a: RouteProps, b: RouteProps) => {
if (a.path !== b.path) {
return false;
}
if (a.query !== b.query) {
return false;
}
return true;
};

const RouterSlot = ({
route,
routerData,
cachedRef,
id,
fallback,
children,
}: {
route: RouteProps;
routerData: RouterData;
cachedRef: MutableRefObject<Record<string, RouteProps>>;
id: string;
fallback?: ReactNode;
children?: ReactNode;
}) => {
const unstable_shouldRenderPrev = (_err: unknown) => {
const shouldSkip = routerData[0];
const skip = getSkipList(shouldSkip, [id], route, cachedRef.current);
const skip = getSkipList(shouldSkip, [id]);
return skip.length > 0;
};
return createElement(
Expand Down Expand Up @@ -329,37 +305,27 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {

const componentIds = getComponentIds(route.path);

const [cached, setCached] = useState<Record<string, RouteProps>>(() => {
return Object.fromEntries(componentIds.map((id) => [id, route]));
});
const [cached, setCached] = useState(() => new Set<string>(componentIds));
const cachedRef = useRef(cached);
useEffect(() => {
cachedRef.current = cached;
}, [cached]);

const changeRoute: ChangeRoute = useCallback(
(route, options) => {
const { checkCache, skipRefetch } = options || {};
const { unstable_checkCache, skipRefetch } = options || {};
startTransition(() => {
setRoute(route);
});
const componentIds = getComponentIds(route.path);
if (
checkCache &&
componentIds.every((id) => {
const cachedLoc = cachedRef.current[id];
return cachedLoc && equalRouteProps(cachedLoc, route);
})
unstable_checkCache &&
componentIds.every((id) => cachedRef.current.has(id))
) {
return; // everything is cached
}
const shouldSkip = routerData[0];
const skip = getSkipList(
shouldSkip,
componentIds,
route,
cachedRef.current,
);
const skip = getSkipList(shouldSkip, componentIds);
if (componentIds.every((id) => skip.includes(id))) {
return; // everything is skipped
}
Expand All @@ -368,14 +334,9 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {
refetch(input, JSON.stringify({ query: route.query, skip }));
}
startTransition(() => {
setCached((prev) => ({
...prev,
...Object.fromEntries(
componentIds.flatMap((id) =>
skip.includes(id) ? [] : [[id, route]],
),
),
}));
// HACK this is just guessing the waku/client behavior
// There should be a better way with exposing something from waku/client
setCached((prev) => new Set([...prev, ...componentIds]));
});
},
[refetch, routerData],
Expand All @@ -385,12 +346,7 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {
(route) => {
const componentIds = getComponentIds(route.path);
const shouldSkip = routerData[0];
const skip = getSkipList(
shouldSkip,
componentIds,
route,
cachedRef.current,
);
const skip = getSkipList(shouldSkip, componentIds);
if (componentIds.every((id) => skip.includes(id))) {
return; // everything is cached
}
Expand All @@ -404,7 +360,9 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {
useEffect(() => {
const callback = () => {
const route = parseRoute(new URL(window.location.href));
changeRoute(route, { checkCache: true });
// TODO unstable_checkCache only works for going back just once
// can we recover state from the History API?
changeRoute(route, { unstable_checkCache: true });
};
window.addEventListener('popstate', callback);
return () => {
Expand Down Expand Up @@ -448,11 +406,7 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {

const children = componentIds.reduceRight(
(acc: ReactNode, id) =>
createElement(
RouterSlot,
{ route, routerData, cachedRef, id, fallback: acc },
acc,
),
createElement(RouterSlot, { routerData, id, fallback: acc }, acc),
null,
);

Expand All @@ -465,7 +419,7 @@ const InnerRouter = ({ routerData }: { routerData: RouterData }) => {

// Note: The router data must be a stable mutable object (array).
type RouterData = [
shouldSkip?: ShouldSkip,
componentConfigs?: ComponentConfigs,
locationListners?: Set<(path: string, query: string) => void>,
];

Expand All @@ -478,24 +432,21 @@ export function Router({ routerData = DEFAULT_ROUTER_DATA }) {
Promise.resolve(data)
.then((data) => {
if (data && typeof data === 'object') {
// We need to process SHOULD_SKIP_ID before LOCATION_ID
if (SHOULD_SKIP_ID in data) {
// TODO replacing the whole array is not ideal
routerData[0] = data[SHOULD_SKIP_ID] as ShouldSkip;
// We need to process COMPONENT_CONFIGS_ID before LOCATION_ID
if (COMPONENT_CONFIGS_ID in data) {
routerData[0] = {
...routerData[0],
...Object.fromEntries(data[COMPONENT_CONFIGS_ID] as any),
};
}
if (LOCATION_ID in data) {
const [pathname, searchParamsString] = data[LOCATION_ID] as [
string,
string,
];
const [pathname, query] = data[LOCATION_ID] as [string, string];
// FIXME this check here seems ad-hoc (less readable code)
if (
window.location.pathname !== pathname ||
window.location.search.replace(/^\?/, '') !== searchParamsString
window.location.search.replace(/^\?/, '') !== query
) {
routerData[1]?.forEach((listener) =>
listener(pathname, searchParamsString),
);
routerData[1]?.forEach((listener) => listener(pathname, query));
}
}
}
Expand Down
13 changes: 4 additions & 9 deletions packages/waku/src/router/common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,11 @@ export function parseInputString(input: string): string {
}

// It starts with "/" to avoid conflicting with normal component ids.
export const SHOULD_SKIP_ID = '/SHOULD_SKIP';
export const COMPONENT_CONFIGS_ID = '/CONFIGS';

// It starts with "/" to avoid conflicting with normal component ids.
export const LOCATION_ID = '/LOCATION';

// TODO revisit shouldSkip API
export type ShouldSkip = (readonly [
componentId: string,
readonly [
path?: boolean, // if we compare path
query?: boolean, // if we compare query
],
])[];
export type ComponentConfigs = {
[componentId: string]: [render?: 'static' | 'dynamic'];
};
14 changes: 7 additions & 7 deletions packages/waku/src/router/create-pages.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,11 @@ export function createPages(
}
return paths;
},
async (id, { unstable_setShouldSkip, unstable_buildConfig }) => {
async (id, { unstable_setComponentConfig, unstable_buildConfig }) => {
await configure(unstable_buildConfig);
const staticComponent = staticComponentMap.get(id);
if (staticComponent) {
unstable_setShouldSkip([]);
unstable_setComponentConfig('static');
return staticComponent;
}
for (const [_, [pathSpec, Component]] of dynamicPagePathMap) {
Expand All @@ -407,12 +407,12 @@ export function createPages(
);
if (mapping) {
if (Object.keys(mapping).length === 0) {
unstable_setShouldSkip();
unstable_setComponentConfig('dynamic');
return Component;
}
const WrappedComponent = (props: Record<string, unknown>) =>
createElement(Component, { ...props, ...mapping });
unstable_setShouldSkip();
unstable_setComponentConfig('dynamic');
return WrappedComponent;
}
}
Expand All @@ -424,7 +424,7 @@ export function createPages(
if (mapping) {
const WrappedComponent = (props: Record<string, unknown>) =>
createElement(Component, { ...props, ...mapping });
unstable_setShouldSkip();
unstable_setComponentConfig('dynamic');
return WrappedComponent;
}
}
Expand All @@ -437,11 +437,11 @@ export function createPages(
if (Object.keys(mapping).length) {
throw new Error('[Bug] layout should not have slugs');
}
unstable_setShouldSkip();
unstable_setComponentConfig('dynamic');
return Component;
}
}
unstable_setShouldSkip([]); // negative cache
unstable_setComponentConfig(); // negative cache
return null; // not found
},
);
Expand Down
31 changes: 13 additions & 18 deletions packages/waku/src/router/define-router.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ import {
getComponentIds,
getInputString,
parseInputString,
SHOULD_SKIP_ID,
COMPONENT_CONFIGS_ID,
LOCATION_ID,
} from './common.js';
import type { RouteProps, ShouldSkip } from './common.js';
import type { RouteProps, ComponentConfigs } from './common.js';
import { getPathMapping } from '../lib/utils/path.js';
import type { PathSpec } from '../lib/utils/path.js';
import { ServerRouter } from './client.js';
Expand All @@ -25,8 +25,6 @@ type RoutePropsForLayout = Omit<RouteProps, 'query'> & {
children: ReactNode;
};

type ShouldSkipValue = ShouldSkip[number][1];

const safeJsonParse = (str: unknown) => {
if (typeof str === 'string') {
try {
Expand Down Expand Up @@ -54,8 +52,9 @@ export function unstable_defineRouter(
getComponent: (
componentId: string, // "**/layout" or "**/page"
options: {
// TODO setShouldSkip API is too hard to understand
unstable_setShouldSkip: (val?: ShouldSkipValue) => void;
unstable_setComponentConfig: (
...args: [render?: 'static' | 'dynamic']
) => void;
unstable_buildConfig: BuildConfig | undefined;
},
) => Promise<
Expand Down Expand Up @@ -117,9 +116,7 @@ export function unstable_defineRouter(
if ((await existsPath(pathname, buildConfig))[0] === 'NOT_FOUND') {
return null;
}
const shouldSkipObj: {
[componentId: ShouldSkip[number][0]]: ShouldSkip[number][1];
} = {};
const componentConfigs: ComponentConfigs = {};

const parsedParams = safeJsonParse(params);

Expand All @@ -135,15 +132,13 @@ export function unstable_defineRouter(
if (skip?.includes(id)) {
return [];
}
const setShoudSkip = (val?: ShouldSkipValue) => {
if (val) {
shouldSkipObj[id] = val;
} else {
delete shouldSkipObj[id];
}
const setComponentConfig = (
...args: [render?: 'static' | 'dynamic']
) => {
componentConfigs[id] = args;
};
const component = await getComponent(id, {
unstable_setShouldSkip: setShoudSkip,
unstable_setComponentConfig: setComponentConfig,
unstable_buildConfig: buildConfig,
});
if (!component) {
Expand All @@ -163,7 +158,7 @@ export function unstable_defineRouter(
}),
)
).flat();
entries.push([SHOULD_SKIP_ID, Object.entries(shouldSkipObj)]);
entries.push([COMPONENT_CONFIGS_ID, Object.entries(componentConfigs)]);
entries.push([LOCATION_ID, [pathname, query]]);
return Object.fromEntries(entries);
};
Expand Down Expand Up @@ -256,7 +251,7 @@ globalThis.__WAKU_ROUTER_PREFETCH__ = (path) => {
export function unstable_redirect(
pathname: string,
query?: string,
skip?: string[],
skip?: string[], // FIXME how could we specify this??
) {
const input = getInputString(pathname);
rerender(input, { query, skip });
Expand Down
Loading

0 comments on commit d5b19bb

Please sign in to comment.