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.

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

refactor: clean up unnecessary props and types, refactor <Route> to always render as children #2995

Merged
merged 2 commits into from
Mar 15, 2023
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
9 changes: 9 additions & 0 deletions .changeset/thirty-countries-happen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
'@commercetools-frontend/application-shell-connectors': patch
'@commercetools-frontend/application-shell': patch
'@commercetools-frontend/permissions': patch
'@commercetools-frontend/constants': patch
'@commercetools-local/visual-testing-app': patch
---

Cleanup unnecessary props and types, refactor `<Route>` to always render using children.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,15 @@ import type { TProviderProps } from '@commercetools-frontend/application-shell-c
import { useIsAuthorized } from '@commercetools-frontend/permissions';
import RouteCatchAll from '../route-catch-all';

type Props<AdditionalEnvironmentProperties extends {}> = {
environment: TProviderProps<AdditionalEnvironmentProperties>['environment'];
type TApplicationEntryPointProps = {
environment: TProviderProps<{}>['environment'];
Comment on lines -10 to +11
Copy link
Member Author

Choose a reason for hiding this comment

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

This type of change is done in many places in this PR.

  • Renamed the type
  • Removed the type argument AdditionalEnvironmentProperties

disableRoutePermissionCheck?: boolean;
render?: () => JSX.Element;
children?: ReactNode;
};

const ApplicationRouteWithPermissionCheck = <
AdditionalEnvironmentProperties extends {}
>(
props: Props<AdditionalEnvironmentProperties>
const ApplicationRouteWithPermissionCheck = (
props: TApplicationEntryPointProps
) => {
const permissionKeys = entryPointUriPathToPermissionKeys(
props.environment.entryPointUriPath
Expand All @@ -33,23 +31,15 @@ const ApplicationRouteWithPermissionCheck = <
return <PageUnauthorized />;
};

const ApplicationRoute = <AdditionalEnvironmentProperties extends {}>(
props: Props<AdditionalEnvironmentProperties>
) => {
const ApplicationRoute = (props: TApplicationEntryPointProps) => {
if (props.disableRoutePermissionCheck) {
return <>{Children.only<ReactNode>(props.children)}</>;
}

return (
<ApplicationRouteWithPermissionCheck<AdditionalEnvironmentProperties>
{...props}
/>
);
return <ApplicationRouteWithPermissionCheck {...props} />;
};

const ApplicationEntryPoint = <AdditionalEnvironmentProperties extends {}>(
props: Props<AdditionalEnvironmentProperties>
) => {
const ApplicationEntryPoint = (props: TApplicationEntryPointProps) => {
// If the `children` prop is used (instead of the `render` prop),
// we pre-configure the application entry point routes to avoid
// users to do so on their own.
Expand All @@ -69,7 +59,7 @@ const ApplicationEntryPoint = <AdditionalEnvironmentProperties extends {}>(
)
}
<Route path={`/:projectKey/${entryPointUriPath}`}>
<ApplicationRoute<AdditionalEnvironmentProperties> {...props} />
<ApplicationRoute {...props} />
</Route>
{/* Catch-all route */}
<RouteCatchAll />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,73 +24,66 @@ import GtmBooter from '../gtm-booter';
import useCoercedEnvironmentValues from './use-coerced-environment-values';
import { getBrowserLocale } from './utils';

type Props<AdditionalEnvironmentProperties extends {}> = {
type TApplicationShellProviderProps = {
apolloClient?: ApolloClient<NormalizedCacheObject>;
environment: TApplicationContext<AdditionalEnvironmentProperties>['environment'];
environment: TApplicationContext<{}>['environment'];
trackingEventList?: TrackingList;
applicationMessages: TAsyncLocaleDataProps['applicationMessages'];
children: (args: { isAuthenticated: boolean }) => JSX.Element;
};

const ApplicationShellProvider = <AdditionalEnvironmentProperties extends {}>(
props: Props<AdditionalEnvironmentProperties>
) => {
const ApplicationShellProvider = (props: TApplicationShellProviderProps) => {
const apolloClient = useMemo(
() => props.apolloClient ?? createApolloClient(),
[props.apolloClient]
);
useEffect(() => {
setCachedApolloClient(apolloClient);
}, [apolloClient]);
const coercedEnvironmentValues =
useCoercedEnvironmentValues<AdditionalEnvironmentProperties>(
props.environment
);
const coercedEnvironmentValues = useCoercedEnvironmentValues(
props.environment
);
const browserLocale = getBrowserLocale(window);
return (
<>
<Suspense fallback={<ApplicationLoader />}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Just moved up...

<ErrorBoundary>
<ApplicationContextProvider<AdditionalEnvironmentProperties>
environment={coercedEnvironmentValues}
>
<ApplicationContextProvider environment={coercedEnvironmentValues}>
<ReduxProvider store={internalReduxStore}>
<ApolloProvider client={apolloClient}>
<Suspense fallback={<ApplicationLoader />}>
Copy link
Member Author

Choose a reason for hiding this comment

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

...from here.

Needed for a follow up PR.

<Router history={ApplicationShellProvider.history}>
<GtmBooter trackingEventList={props.trackingEventList || {}}>
<ApplicationPageTitle />
<Authenticated
locale={browserLocale}
applicationMessages={props.applicationMessages}
render={({ isAuthenticated }) => {
if (isAuthenticated)
return props.children({ isAuthenticated });
<Router history={ApplicationShellProvider.history}>
<GtmBooter trackingEventList={props.trackingEventList || {}}>
<ApplicationPageTitle />
<Authenticated
locale={browserLocale}
applicationMessages={props.applicationMessages}
render={({ isAuthenticated }) => {
if (isAuthenticated)
return props.children({ isAuthenticated });

return (
<AsyncLocaleData
locale={browserLocale}
applicationMessages={props.applicationMessages}
>
{({ locale, messages }) => (
<ConfigureIntlProvider
locale={locale}
messages={messages}
>
{props.children({ isAuthenticated })}
</ConfigureIntlProvider>
)}
</AsyncLocaleData>
);
}}
/>
</GtmBooter>
</Router>
</Suspense>
return (
<AsyncLocaleData
locale={browserLocale}
applicationMessages={props.applicationMessages}
>
{({ locale, messages }) => (
<ConfigureIntlProvider
locale={locale}
messages={messages}
>
{props.children({ isAuthenticated })}
</ConfigureIntlProvider>
)}
</AsyncLocaleData>
);
}}
/>
</GtmBooter>
</Router>
</ApolloProvider>
</ReduxProvider>
</ApplicationContextProvider>
</ErrorBoundary>
</>
</Suspense>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,14 @@ const shallowlyCoerceValues = (uncoercedEnvironmentValues: ShallowJson) =>
{}
);

const useCoercedEnvironmentValues = <
AdditionalEnvironmentProperties extends {}
>(
const useCoercedEnvironmentValues = (
environment: ShallowJson
): TApplicationContext<AdditionalEnvironmentProperties>['environment'] => {
): TApplicationContext<{}>['environment'] => {
const coercedEnvironmentValues = useMemo(
() =>
shallowlyCoerceValues(
environment
) as TApplicationContext<AdditionalEnvironmentProperties>['environment'],
) as TApplicationContext<{}>['environment'],
[environment]
);
return coercedEnvironmentValues;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import GlobalStyles from './global-styles';
import RedirectToLogin from './redirect-to-login';
import RedirectToLogout from './redirect-to-logout';

type Props<AdditionalEnvironmentProperties extends {}> = {
type TApplicationShellProps = {
apolloClient?: ApolloClient<NormalizedCacheObject>;
/**
* NOTE: the environment value passed here is usually `window.app`.
Expand All @@ -75,7 +75,7 @@ type Props<AdditionalEnvironmentProperties extends {}> = {
* <ApplicationShell environment={window.app} />
* ```
*/
environment: TApplicationContext<AdditionalEnvironmentProperties>['environment'];
environment: TApplicationContext<{}>['environment'];
featureFlags?: TFlags;
defaultFeatureFlags?: TFlags;
trackingEventList?: TrackingList;
Expand Down Expand Up @@ -139,17 +139,15 @@ export const MainContainer = styled.main`
position: relative;
`;

export const RestrictedApplication = <
AdditionalEnvironmentProperties extends {}
>(
export const RestrictedApplication = (
props: Omit<
Props<AdditionalEnvironmentProperties>,
TApplicationShellProps,
'environment' | 'onRegisterErrorListeners'
>
) => {
const applicationEnvironment = useApplicationContext(
(context) => context.environment
) as TApplicationContext<AdditionalEnvironmentProperties>['environment'];
) as TApplicationContext<{}>['environment'];
// TODO: using this hook will subscribe the component to route updates.
// This is currently useful for detecting a change in the project key
// from URL ("/" --> "/:projectKey").
Expand Down Expand Up @@ -213,7 +211,7 @@ export const RestrictedApplication = <

const projectKeyFromUrl = selectProjectKeyFromUrl(location.pathname);
return (
<ApplicationContextProvider<AdditionalEnvironmentProperties>
<ApplicationContextProvider
user={user}
environment={applicationEnvironment}
>
Expand Down Expand Up @@ -301,7 +299,7 @@ export const RestrictedApplication = <
locale: dataLocale,
setProjectDataLocale,
}) => (
<ApplicationContextProvider<AdditionalEnvironmentProperties>
<ApplicationContextProvider
user={user}
project={project}
projectDataLocale={dataLocale}
Expand Down Expand Up @@ -369,7 +367,7 @@ export const RestrictedApplication = <
return <LoadingNavBar />;

return (
<ApplicationContextProvider<AdditionalEnvironmentProperties>
<ApplicationContextProvider
user={user}
environment={applicationEnvironment}
// NOTE: do not pass the `project` into the application context.
Expand Down Expand Up @@ -460,10 +458,8 @@ export const RestrictedApplication = <
}
</Route>
{/* Project routes */}
<Route
exact={true}
path="/"
render={() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

No more render or component prop, routes are rendered as children of <Route>

<Route exact={true} path="/">
{(() => {
const previousProjectKey =
getPreviousProjectKey(
user?.defaultProjectKey ?? undefined
Expand All @@ -483,29 +479,23 @@ export const RestrictedApplication = <
return (
<Redirect to={`/${previousProjectKey}`} />
);
}}
/>
<Route
exact={false}
path="/:projectKey"
render={(routerProps) => (
<ProjectContainer<AdditionalEnvironmentProperties>
user={user}
match={routerProps.match}
location={routerProps.location}
environment={applicationEnvironment}
disableRoutePermissionCheck={
props.disableRoutePermissionCheck
}
// This effectively renders the
// children, which is the application
// specific part
render={props.render}
>
{props.children}
</ProjectContainer>
)}
/>
})()}
</Route>
<Route exact={false} path="/:projectKey">
<ProjectContainer
user={user}
environment={applicationEnvironment}
disableRoutePermissionCheck={
props.disableRoutePermissionCheck
}
// This effectively renders the
// children, which is the application
// specific part
render={props.render}
>
{props.children}
</ProjectContainer>
</Route>
</Switch>
</div>
</MainContainer>
Expand All @@ -524,9 +514,7 @@ export const RestrictedApplication = <
};
RestrictedApplication.displayName = 'RestrictedApplication';

const ApplicationShell = <AdditionalEnvironmentProperties extends {}>(
props: Props<AdditionalEnvironmentProperties>
) => {
const ApplicationShell = (props: TApplicationShellProps) => {
useEffect(() => {
props.onRegisterErrorListeners?.({
dispatch: internalReduxStore.dispatch,
Expand All @@ -537,7 +525,7 @@ const ApplicationShell = <AdditionalEnvironmentProperties extends {}>(
return (
<>
<GlobalStyles />
<ApplicationShellProvider<AdditionalEnvironmentProperties>
<ApplicationShellProvider
apolloClient={props.apolloClient}
environment={props.environment}
trackingEventList={props.trackingEventList}
Expand All @@ -551,7 +539,7 @@ const ApplicationShell = <AdditionalEnvironmentProperties extends {}>(
<RedirectToLogout />
</Route>
<Route>
<RestrictedApplication<AdditionalEnvironmentProperties>
<RestrictedApplication
defaultFeatureFlags={props.defaultFeatureFlags}
featureFlags={props.featureFlags}
render={props.render}
Expand Down
Loading