Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Font optimizations #14746
Font optimizations #14746
Changes from 15 commits
eff6343
9ed7962
3b85140
3934f53
d5ed404
39fcfcd
8a932d1
126f904
069cbfa
27bdbfe
6a14652
d5137c3
7a79523
0f89a6c
2f45a7b
6f9e1e7
210a5cf
ca93eba
0bad5cb
463ca85
004ebcb
6acdce5
2289980
0d0c96d
45e7929
2602b31
5cde0b2
fcf567c
f70f936
3ef9d55
98e41c7
b8917c5
0dabf09
9bfd563
e17d425
c397f6c
5f7ddd1
a0a9c14
c7fe9e9
a21bba1
d2be443
b7cd379
05cc713
623a965
dc83f9d
1e23ca3
97f89fe
cb3ff87
3eaade2
6000fae
20ab602
91b5bb4
cb5e320
cf4d9d1
bf09fcb
bd2f156
815d279
58ad186
8de7630
6d600ab
477937b
a462650
3762863
af2c9d6
45a90ed
b3f7aea
03eb098
7f5680a
2d77d04
90f41fb
0b6ee6b
eeb5375
9f7f904
eb7f10e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I believe you can tap into the callExpression directly instead of manually traversing the AST:
https://github.com/webpack/webpack/blob/master/lib/javascript/JavascriptParser.js#L236-L260
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.
I agree. This might also let us remove the logic from both
next/head
as we could just make the link tags inert here itself.But I could never get this to work hence added
recast
at the time of conformance work.FWIW, this is the code i tried
^ would let us capture the call creating the link tag
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.
Update:https://twitter.com/_prateekbh/status/1280603310211215361
Done 👍🏻
its not the most readable/straightforward thing, but it'll definitely be faster
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.
Instead of adding a new manifest it'd easier to reuse the build-manifest I think 🤔
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.
May be, however I am afraid that by writing
build-manifest
from multiple plugins they might overwrite each other?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.
It doesn't have to be multiple plugins though, we can share information into the build manifest 👍
The reason using build manifest is better is that it's automatically made available to rendering and _document, meaning you don't have to change the rendering code as much as you have to when introducing a new file
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.
I found that BuildManifestPlugin is built on client run of webpack. This is a problem because FontStylesheetGatheringPlugin runs on the server build of webpack.
The reason for keeping it on server side run is that otherwise it does not build the user defined
_document.jsx
which is a common place to define stylesheets and fonts and seems to be building only for server run of webpack.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.
@timneutkens would there be a way to generate this on server run and consolidate during client bundle of webpack?