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

Use git hash to break cache #1684

Merged
merged 15 commits into from Jun 29, 2023
Merged

Use git hash to break cache #1684

merged 15 commits into from Jun 29, 2023

Conversation

SleeplessOne1917
Copy link
Member

Closes #1676

src/server/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@alectrocute alectrocute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending @sunaurus’s feedback and the missing new line at the end of one of those files, this looks great and solves a huge bug. Fantastic!

@alectrocute
Copy link
Contributor

Also, epic rhyme in PR title.

@alectrocute
Copy link
Contributor

What’s going on with CI here? Definitely would be great to get this in the next RC.

webpack.config.js Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
@@ -96,15 +101,15 @@ const createClientConfig = (_env, mode) => {
entry: "./src/client/index.tsx",
output: {
filename: "js/client.js",
publicPath: "/static/",
publicPath: `/static/${commitHash}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
publicPath: `/static/${commitHash}`
publicPath: `/static/${commitHash}/`

Without the trailing slash the paths break:

Unhandled Promise Rejection: ChunkLoadError: Loading chunk 76292 failed.
(error: http://localhost:1234/static/3caf913614868f70475430fca3dcc40c61d613b5js/76292.client.js)

@sunaurus
Copy link
Collaborator

sunaurus commented Jun 29, 2023

This is totally optional, but IMO using the --short git hashes will be a bit nicer while still giving absurdly low chances of collisions.

With git rev-parse HEAD: http://localhost:1234/static/3caf913614868f70475430fca3dcc40c61d613b5/js/76292.client.js

With git rev-parse --short HEAD: http://localhost:1234/static/3caf913/js/76292.client.js

@alectrocute
Copy link
Contributor

Sorry to complicate things, but couldn't we just do cache busting without git, eg. webpack config?

@sunaurus
Copy link
Collaborator

I think the issue with webpack hashes is that we run the server and client webpack builds seperately, and I'm not sure if there's any clean way to take the resulting hash(es) from the client build, and replace them into imports in the generated server files. My assumption is that it would require a bigger refactor of the webpack configuration. But I'm no webpack expert, maybe there's some obvious way to do it that I'm missing!

@alectrocute
Copy link
Contributor

I think the issue with webpack hashes is that we run the server and client webpack builds seperately, and I'm not sure if there's any clean way to take the resulting hash(es) from the client build, and replace them into imports in the generated server files. My assumption is that it would require a bigger refactor of the webpack configuration. But I'm no webpack expert, maybe there's some obvious way to do it that I'm missing!

I think there's a way to export the contenthash that webpack generates and export it as an env variable to be used by the server, but I'm also not an expert in webpack. :/

@dessalines
Copy link
Member

@SleeplessOne1917 @sunaurus Ya that's an easy one to fix:

In the .woodpecker.yml, add this before the yarn build:dev

- apk add git

@dessalines
Copy link
Member

dessalines commented Jun 29, 2023

Okay actually I'm kinda changing my mind a bit:

Check out this line: https://github.com/LemmyNet/lemmy-ui/blob/main/Dockerfile#L27

It should also set the commit hash there.

Why not just let the webpack read the version / commit hash from the version.ts file? The UI, the build, everything reads from that version.ts file as the source of truth.

@SleeplessOne1917
Copy link
Member Author

I think the issue with webpack hashes is that we run the server and client webpack builds seperately, and I'm not sure if there's any clean way to take the resulting hash(es) from the client build, and replace them into imports in the generated server files. My assumption is that it would require a bigger refactor of the webpack configuration. But I'm no webpack expert, maybe there's some obvious way to do it that I'm missing!

Pretty much this. The built in hashes for webpack were the first thing I tried, but I couldn't figure out how to use them in the actual code.

@dessalines

Check out this line: https://github.com/LemmyNet/lemmy-ui/blob/main/Dockerfile#L27
It should also set the commit hash there.

See the most recent commit I made. Using the hash is now docker agnostic, and it's set as an environment variable.

src={avatar ?? "/static/assets/icons/icon-96x96.png"}
src={
avatar ??
`/static/${process.env.COMMIT_HASH}/assets/icons/icon-96x96.png`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this into a bundleFolder() or bundleDir() function? That way we won't have to trace all these down again potentially in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. I vote for something like getStaticDir and have it in @utils/env.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the change during my lunch break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the changes.

@alectrocute alectrocute added this to the 0.18.1 milestone Jun 29, 2023
@dessalines dessalines merged commit 7514957 into main Jun 29, 2023
2 checks passed
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.

Implement cache invalidation features
4 participants