-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: code split & dynamic imports #188
Conversation
…js' into fix/dynamic-imported-ui-components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self review
<Suspense fallback={<div>Loading...</div>}> | ||
{currentRoute?.component != null && <currentRoute.component />} | ||
</Suspense> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could avoid any potential "not prod ready" issues with preact's Suspense
by doing a simple useEffect wrapper of the async operation
export const ConfigProvider = ({ children, expanded = isLoadedInIframe }: { children: JSX.Element[] | JSX.Element, expanded?: boolean }): JSX.Element => { | ||
const [isConfigExpanded, setConfigExpanded] = useState(expanded) | ||
export const ConfigProvider = ({ children }: { children: JSX.Element[] | JSX.Element, expanded?: boolean }): JSX.Element => { | ||
const [isConfigExpanded, setConfigExpanded] = useState(isConfigPage(window.location.hash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be updated to loaded dynamically but I don't think it would give us much for the effort
gotoPage(route?: string): void | ||
}>({ currentRoute: undefined, gotoPage: () => {} }) | ||
|
||
export const RouterProvider = ({ children, routes }: { children: React.ReactNode, routes: Route[] }): JSX.Element => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very basic hash-router implementation. I looked around for some and most were 200kb and unmaintained, or very large. All required some intimate knowledge or specific finagling for things we needed.
lmk if there's something I missed.
import Form from '../components/Form.jsx' | ||
import Header from '../components/Header.jsx' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: rename these to snake-case, form.jsx
and header.jsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took this for a spin and works as expected, very cool improvement for initial and subdomain loads ❤️
Once this lands, it will be a good time to merge the new release too (#121), feel free to ship both!
Title
feat: code split & dynamic imports
Description
Implements a version of #93 (comment).
Fixes #93
Fixes #147
FYI that this conflicts slightly with #182 and differences need to be resolved (should be a minor change)that PR was merged into this oneSummary of changes:
Notes & open questions
NOTE: This PR is in draft because serving different JS bundles based on subdomain is a little tricky, but I will look into that now that I have basic code splitting and dynamic loading working with decent pure routing functions.
Details about assets
What's pulled on first hit
The below are loaded in firefox using a server ran with:
> npx http-server -p 3335 dist
main domain
http://localhost:3335
14 requests
1.11 MB / 1.04 MB transferred
Finish: 113 ms
DOMContentLoaded: 12 ms
load: 56 ms
main domain
http://localhost:3335/#/ipfs-sw-config
2 requests
35.47 kB / 36.11 kB transferred
Finish: 14 ms
subdomain
http://specs-ipfs-tech.ipns.localhost:3335
27 requests
2.13 MB / 1.99 MB transferred
Finish: 233 ms
DOMContentLoaded: 16 ms
load: 72 ms
Note that the config page in the iframe makes additional requests for the same assets, and I have cache disabled.
Change checklist