-
Notifications
You must be signed in to change notification settings - Fork 798
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
Instant Search: Lazy load #19049
Instant Search: Lazy load #19049
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code. jetpack plugin:
|
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.
Great changes all around. The loader bundle is now 2.31 kB gzipped, which is an order of magnitude better than loading the entire application bundle at once (46.05 kB gzipped).
This change as-is breaks our bundle size limit testing, unfortunately (yarn test-search-size
). Once that's squared away, this PR should be good to go.
projects/plugins/jetpack/modules/search/instant-search/loader.js
Outdated
Show resolved
Hide resolved
It should be taken care of now 👍 |
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.
Changes look good, but no search bundles are being loaded at the moment due to this conditional; this is happening due to _inc/build/instant-search/jp-search-main.bundle.css
no longer existing.
I think deleting all mentions of this bundled CSS like so should fix this regression.
Just an aside: EDIT: Noted in p3topS-Sn-p2. |
Good catch, thanks @jsnmoon! I've been testing this in wpcom, and that conditional doesn't seem to be present there. This should now be fixed. |
d62e30b
to
0929d5b
Compare
Rebased. |
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.
Great work! 🚢
Thank you, @jsnmoon! I was going to commit, but it seems that the patch can't apply cleanly to wpcom (probably because of the aforementioned differences), which is blocking the PR. What do you think would be the best way forward? It's not clear to me whether the different code in wpcom is intentional or the result of some sort of syncing flaw. |
Caution: This PR has changes that must be merged to WordPress.com |
I went ahead and manually sorted out the patch conflicts for D58433-code. It should be good to go, but some smoke-testing before shipping might be a good idea! Aside: I'd consider the mismatch between Jetpack code and WPCOM code to be tech debt and will likely need to be ironed out in the near future. |
I checked and everything was good 👍 Adding the changelog, which is now mandatory, seems to have invalidated the previous review. I wish that check was smarter, as a changelog file is clearly not code and doesn't need a review. In order to move things along, I deployed the wpcom change. I'm not sure if that was a good idea, as now the Sorry for all the extra work. The Jetpack deployment process is very complex, and I haven't fully wrapped my head around it 😕 |
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.
Happy to move things along, this is a great change! Forced the WPCOM check to true via MC's fusion tool.
@sgomes shipped to WPCOM in r222614-wpcom. |
Great news! One last step: head over to your WordPress.com diff, D58433-code, and commit it. Thank you! |
Instant Search is currently served up as a (mostly) single bundle, with an additional polyfill chunk that only gets loaded for IE 11. Because of the way JS loading is traditionally done in WordPress, this is loaded as part of the critical path, delaying first render and page interactivity.
This PR explores one approach for changing this paradigm, by loading a much smaller amount of code in the critical path. It accomplishes this by moving most of the code to a dynamic import that only runs on
DOMContentLoaded
(i.e., when the document is done parsing).This results in an order of magnitude less JavaScript in the critical path (from somewhere in the 40KB gzipped range to somewhere in the 2KB gzipped range) for the Instant Search feature, with relatively minor changes to the JS code and
nominor changes needed to the PHP side of things.Incidentally, the benefits extend to CSS as well, which automatically gets split and loaded similarly.
Changes proposed in this Pull Request:
Jetpack product discussion
None.
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
https://example.com/?s=query
) correctly pops up Instant Search on open.I tested this on my
wpcom
sandbox, so it may be a good idea for reviewers to try it out on e.g. a local install.Proposed changelog entry for your changes: