-
Notifications
You must be signed in to change notification settings - Fork 687
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
[feature]: Lazy Load/trim unused bytes in main routes #2988
Changes from 3 commits
589fa24
de85854
230398b
8301136
feadfb2
8e671c6
90d993a
2e059ae
e19c6d1
0b8d254
009da4d
7fc44bc
43dd4df
dc5e8d1
cdd2b0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
const react = jest.requireActual('react'); | ||
|
||
module.exports = { | ||
...react, | ||
lazy: fn => fn.toString() | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,29 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`displays open nav or drawer 1`] = ` | ||
<div> | ||
Title | ||
<Main | ||
isMasked={false} | ||
> | ||
<Routes /> | ||
</Main> | ||
<button | ||
onClick={[Function]} | ||
/> | ||
<() => { | ||
/* istanbul ignore next */ | ||
cov_101e9trynu().f[0]++; | ||
cov_101e9trynu().s[1]++; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why code coverage started showing up in some of the snapshots but not others. It has something to do with the jest mock. When the mock is local to the component being tested the coverage comments do not appear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok it looks like this was happening because I was running with some jest filter like "pattern" or "failed test" which caused the reporter to attach comments for that run. When I run the entire suite, they are removed. |
||
return ( | ||
/* istanbul ignore next */ | ||
Promise.resolve().then(() => _interopRequireWildcard(require('../Navigation'))) | ||
); | ||
} /> | ||
<ToastContainer /> | ||
</div> | ||
`; | ||
|
||
exports[`renders with renderErrors 1`] = ` | ||
<HeadProvider> | ||
<Title> | ||
|
@@ -29,7 +53,19 @@ exports[`renders with unhandledErrors 1`] = ` | |
dismiss={[Function]} | ||
isActive={false} | ||
/> | ||
<Navigation /> | ||
<React.Suspense | ||
fallback={null} | ||
> | ||
<() => { | ||
/* istanbul ignore next */ | ||
cov_101e9trynu().f[0]++; | ||
cov_101e9trynu().s[1]++; | ||
return ( | ||
/* istanbul ignore next */ | ||
Promise.resolve().then(() => _interopRequireWildcard(require('../Navigation'))) | ||
); | ||
} /> | ||
</React.Suspense> | ||
<ToastContainer /> | ||
</HeadProvider> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ import { useAppContext } from '@magento/peregrine/lib/context/app'; | |
|
||
import Main from '../../Main'; | ||
import Mask from '../../Mask'; | ||
import Navigation from '../../Navigation'; | ||
import Routes from '../../Routes'; | ||
|
||
const renderer = new ShallowRenderer(); | ||
|
@@ -15,7 +14,7 @@ jest.mock('../../Head', () => ({ | |
Title: () => 'Title' | ||
})); | ||
jest.mock('../../Main', () => 'Main'); | ||
jest.mock('../../Navigation', () => 'Navigation'); | ||
|
||
jest.mock('../../Routes', () => 'Routes'); | ||
jest.mock('../../ToastContainer', () => 'ToastContainer'); | ||
|
||
|
@@ -160,7 +159,8 @@ test('renders a full page with onlineIndicator and routes', () => { | |
}; | ||
const { root } = createTestInstance(<App {...appProps} />); | ||
|
||
getAndConfirmProps(root, Navigation); | ||
// TODO: Figure out how to mock the React.lazy call to export the component | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey dude, is this still valid given you have added a react mock in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. There is no |
||
// getAndConfirmProps(root, Navigation); | ||
|
||
const main = getAndConfirmProps(root, Main, { | ||
isMasked: false | ||
|
@@ -244,9 +244,8 @@ test('displays open nav or drawer', () => { | |
unhandledErrors: [] | ||
}; | ||
|
||
const { root: openNav } = createTestInstance(<App {...props} />); | ||
|
||
getAndConfirmProps(openNav, Navigation); | ||
const root = createTestInstance(<App {...props} />); | ||
expect(root.toJSON()).toMatchSnapshot(); | ||
}); | ||
|
||
test('renders with renderErrors', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import React from 'react'; | ||
import React, { Suspense } from 'react'; | ||
import { useIntl } from 'react-intl'; | ||
|
||
import { Section } from '../../Accordion'; | ||
import GiftCards from '../GiftCards'; | ||
|
||
const GiftCards = React.lazy(() => import('../GiftCards')); | ||
|
||
const GiftCardSection = props => { | ||
const { setIsCartUpdating } = props; | ||
|
@@ -15,7 +16,9 @@ const GiftCardSection = props => { | |
defaultMessage: 'Apply Gift Card' | ||
})} | ||
> | ||
<GiftCards setIsCartUpdating={setIsCartUpdating} /> | ||
<Suspense fallback={null}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add loading spinners. I just didn't think it was necessary since all the sections are behind user interaction. You're right though, we should probably include something to indicate loading is happening. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intended to be a conversation starter, don't implement anything yet. I honestly haven't been able to catch an unloaded suspense component, even on Slow 3G, so if the use case isn't realistic, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's easy enough to add them to these price adjustment accordions. Everything else though, I will not touch, including modals, or dropdowns, or the left nav. |
||
<GiftCards setIsCartUpdating={setIsCartUpdating} /> | ||
</Suspense> | ||
</Section> | ||
); | ||
}; | ||
|
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.
For starters, I might have scope creeped this a bit but as far as I can tell we no longer use the
templates
, opting instead to usevenia-concept/template.html
. @zetlen @revanth0212 can ya'll confirm this?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.
You are right. We got rid of mustache templates in favor of Webpack's HTML Plugin which utilized template.html to render the main HTML file.