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

Config-based routing, wouter v3, custom useLocation() / navigate() #556

Merged
merged 54 commits into from
Dec 10, 2024

Conversation

Danziger
Copy link
Contributor

@Danziger Danziger commented Nov 29, 2024

  • Extract page wrapper elements/styles to a new Page component.
  • Rename all "view components" to <Name>View and replace default exports with named exports.
  • Create a HOC (withPage) to wrap "views" with the default Page component.
  • Set up config-based routing, that automatically wraps views in the provided Page component.
  • Create new <Routes> component that renders the route config objects, and use it to all apps, including the Welcome and the Dashboard apps (but only part of their routing uses it, the rest is still done by manually checking params).
  • Create useLocation() and useSearchParam() hooks that wrap and extend wouter's useLocation(), useSearch() and navigate() functions
  • Get rid of the old <HistoryProvider> component and useHistory() hook.
  • Add types for route paths (ArConnectRoutePath) and use those in navigate(), <Redirect> and <Link>.
  • Update wouter to the latest version.
  • Closes Router issues #510.
  • Closes Routing: Search params are manually parsed #560.
  • Add a <WalletsProvider> and useWallets() hook to replace the logic inside src/wallets/setup/browser-extension/browser-extension-wallet-setup.hook.ts and get rid of the old useNoWallets() and useDecryptionKey() hooks (which were not reactive).

@Danziger Danziger marked this pull request as draft November 29, 2024 14:58
src/popup.tsx Outdated
</HistoryProvider>
</Router>
<>
<Routes routes={POPUP_ROUTES} />
Copy link
Contributor Author

@Danziger Danziger Nov 29, 2024

Choose a reason for hiding this comment

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

Routes have been defined in POPUP_ROUTES and are wrapped automatically in a Page component by the Routes component.

The params are automatically passed as params prop to all of them, so there's no need to wrap them in functions like we had before.

Additionally, the Head component could be extracted here (just like NavigationBar) so that it is not affected by page transitions, but that would also require adding a headProps property to the RouterConfig interface (for static personalization) and also creating a context and/or portal to allow for personalization coming directly from the view/page's own life cycle (for dynamic personalization).

@@ -84,6 +86,30 @@ export function AuthRequestsProvider({
[]
);

const closeAuthPopup = useCallback((delay: number = 0) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small change for the embedded wallet already here, but it doesn't affect the functionality in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -232,6 +134,12 @@ export async function setActiveWallet(address?: string) {
export type DecryptedWallet = StoredWallet<JWKInterface>;

export async function openOrSelectWelcomePage(force = false) {
if (process.env.PLASMO_PUBLIC_APP_TYPE !== "extension") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor change for the embedded wallet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Already added env var to GH repo 👍

active={page === i + 1}
onClick={() => navigateToPage(i + 1)}
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 could be replaced with navigate(i + 1). See #568

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

import type { CommonRouteProps } from "~wallets/router/router.types";
import { Redirect } from "~wallets/router/components/redirect/Redirect";

const Views = [
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 could/should be replaced with <Routes> too. See #567.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree


// Wallet generate pages:

// TODO: Use a nested router instead:
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 could/should be implemented using <Routes>. See #567.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


if (![1, 2, 3].includes(page)) return 1;
const Views = [
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 could/should be implemented using <Routes>. See #567.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

<Page
key={i}
active={page === i + 1}
onClick={() => navigate(`/start/${i + 1}`)}
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 could/should be replaced with navigate(i+1). See #568.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


const currentLocation = () => window.location.hash.replace(/^#/, "") || "/";

export const useHashLocation: BaseLocationHook = () => {
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 comes from wouter now.

authType?: RouteAuthType;
}

export type ArConnectRoutePath =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Routes (across all apps) are now type-safe, which includes calls to navigate(), <Link> and <Redirect>.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

key?: string;
path: P;
component: React.ComponentType<CommonRouteProps>;
authType?: RouteAuthType;
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 param will probably be used for embedded.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return null;
}

export type NavigateAction = "prev" | "next" | "up" | number;
Copy link
Contributor Author

@Danziger Danziger Dec 4, 2024

Choose a reason for hiding this comment

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

Shortcuts for common routing actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍


// TODO: Consider adding `<AnimatePresence>` and `<BodyScroller>` inside here.

// TODO: Consider adding a prop to `RouteConfig.parseParams` to parse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might do it later. Removes the need to parse params in the views, as parsing would be done at the router level.

})}
</Switch>
);
}, [routes, diffLocation ? location : undefined]);
Copy link
Contributor Author

@Danziger Danziger Dec 4, 2024

Choose a reason for hiding this comment

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

Diffing the location makes different views that use the same component to show a transition between the two (e.g. selecting an AuthRequest of the same type in the Auth popup). This is off by default (as that's how it was working before).

Copy link
Contributor

Choose a reason for hiding this comment

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

What are we waiting to turn it on? 🙌

@Danziger Danziger changed the title Config based router Config-based routing and custom useLocation() / navigate() Dec 4, 2024
@Danziger Danziger changed the title Config-based routing and custom useLocation() / navigate() Config-based routing, wouter v3, custom useLocation() / navigate() Dec 4, 2024
- Move <AnimatePresence> and <BodyScroller> into <Routes>.
- Fix rendering issue in <HeadAuth> making it re-render constantly.
- Implement routing overrides.
- Use routing overrides for the cover, loading, unlock and unlocked states of the Popup and Auth apps.
- Get rid of useNowallets() and useDecryptionKey() in favour of the new useWallets() hook and <WalletsProvider>.
@Danziger
Copy link
Contributor Author

Danziger commented Dec 6, 2024

@7i7o A PLASMO_PUBLIC_APP_TYPE = "extension" env variable is needed after merging these changes.

});
}

loadAppInfo();
}, [url, appInfoProp]);
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 was causing an infinite rendering loop on this component.


useEffect(() => {
(async () => {
const activeAddress = await getActiveAddress();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not ideal because the state won't change if the wallets are removed. Same with the useDecryptionKey hook below. Both have been replaced with useWallets() which gets the state from <WalletsProvider> which subscribes to state updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Danziger Danziger mentioned this pull request Dec 9, 2024
4 tasks
Copy link
Contributor

@7i7o 7i7o left a comment

Choose a reason for hiding this comment

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

tACK 👍

@@ -93,7 +92,7 @@
"uuid": "^9.0.0",
"warp-contracts": "^1.2.13",
"webextension-polyfill": "^0.10.0",
"wouter": "^2.8.0-alpha.2"
"wouter": "^3.3.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

export type ApplicationsDashboardViewProps =
CommonRouteProps<ApplicationsDashboardViewParams>;

export function ApplicationsDashboardView() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🦸

isQuickSetting
}: AddContactDashboardViewProps) {
const { navigate } = useLocation();
const { address } = useSearchParams<{ address: string }>();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

const searchInput = useInput();

// active setting val
const activeSetting = useMemo(() => params.setting, [params.setting]);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return undefined;
}

return new Application(decodeURIComponent(activeSubSetting));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

authType?: RouteAuthType;
}

export type ArConnectRoutePath =
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}, [routes]);
}

const memoizedRoutes = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const memoizedRoutes = useMemo(() => {
const memorizedRoutes = useMemo(() => {

typo

})}
</Switch>
);
}, [routes, diffLocation ? location : undefined]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we waiting to turn it on? 🙌

<>
<BodyScroller />

<AnimatePresence initial={false}>{memoizedRoutes}</AnimatePresence>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<AnimatePresence initial={false}>{memoizedRoutes}</AnimatePresence>
<AnimatePresence initial={false}>{memorizedRoutes}</AnimatePresence>

same typo

return null;
}

export type NavigateAction = "prev" | "next" | "up" | number;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Danziger Danziger merged commit f51ae70 into development Dec 10, 2024
@Danziger Danziger deleted the feature/config-based-router branch December 10, 2024 15:23
Copy link

🎉 This PR is included in version 1.21.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants