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

cache bust index html on arbitrary dom changes #1474

Conversation

void-mAlex
Copy link
Collaborator

@void-mAlex void-mAlex commented Jun 20, 2023

what this pr introduces is a cache key in the form of a render on first cache of the appInfo
subsequent runs render the html file before it would be processed and altered by webpack to contain all chunks this providing a stable key to check against
we also have a final check in place once webpack has built to not write a file if the final content is unchanged which is done as a file system check

@ef4 this seems to be caused by the way we check if appInfo is the same and if we need to set up a new webpack instance
I did not find a better way to query at start of the build or at another point during the process

@void-mAlex void-mAlex changed the title render on first cache and compare with re-renders of subsequent runs cache bust index html on arbitrary dom changes Jun 20, 2023
@void-mAlex void-mAlex requested a review from ef4 June 20, 2023 18:41
@ef4
Copy link
Contributor

ef4 commented Jun 20, 2023

But do we need a new webpack config? Just changing content in the HTML should not require that.

We already compare the output of HTMLEntrypoint.render from the previous run here. This is supposed to accomplish the same thing that you're doing with simpleRender.

I suspect that your fix works by invalidating more than we really need to invalidate and that there's still something more targeted that is going wrong.

@void-mAlex
Copy link
Collaborator Author

void-mAlex commented Jun 21, 2023

But do we need a new webpack config? Just changing content in the HTML should not require that.

short answer is no, I've also tested that it's fixed if I replace the entrypoints array on lastAppInfo and let it run like that it works but that seems riskier as it feels like missing out on side effects that happens in configureWebpack to entrypoints.dom the version that goes into the function as appInfo and the version that is seen by write files is different and that makes me consider there may be others as well that I'm not aware of.

We already compare the output of HTMLEntrypoint.render from the previous run here. This is supposed to accomplish the same thing that you're doing with simpleRender.

the content that is seen at that point is from the first cached lastAppInfo.entrypoints.dom which is what this tries to fix as that will never go into the if you pointed out due to correctly being the same at that point but not the one that want meaning the updated dom from newly examined appInfo.entrypoints.dom

what simpleRender sees is before webpack processes it thus without any of the hashes making it more stable as a cache key

I suspect that your fix works by invalidating more than we really need to invalidate and that there's still something more targeted that is going wrong.

I thought so as much as well (that it may be heavy handed to do this) but can't see anything more targeted than this that also caters to not change the semantics of code around it. the change as far as I was able to target it will only trigger when the content of either one of your entry points gets changes without caring for the changes that webpack (or us) do to the file in terms of number of chunks present or link tags

@void-mAlex
Copy link
Collaborator Author

@ef4
specifically

if (this.lastWebpack && this.lastAppInfo && equalAppInfo(appInfo, this.lastAppInfo)) {

appInfo.entrypoints[0].dom is the correct updated html but because the equalAppInfo doesn't take that into account so it considers the lastAppInfo valid and this.lastAppInfo.entrypoints[0].dom does not have any of the updated html

ef4 added a commit that referenced this pull request Sep 12, 2023
This is an alternate fix for #1474
@ef4
Copy link
Contributor

ef4 commented Sep 12, 2023

Closing in favor of #1597

@ef4 ef4 closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants