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

Asset change should only emit when the file contents have changed #60

Closed
skinandbones opened this issue Aug 1, 2017 · 17 comments
Closed
Assignees

Comments

@skinandbones
Copy link

An asset change is being triggered when the file mtime changes with no change in content. This can cause unexpected page reloads. To reproduce,

iex(2)> :crypto.hash(:md5, File.read!("priv/static/css/app.css")) |> Base.encode16
"1C16A9E2D001DDFF3292FC7C4EE9F89E"

iex(3)> File.touch("priv/static/css/app.css")
:ok

[debug] Live reload: priv/static/css/app.css
[debug] Live reload: priv/static/css/app.css

iex(5)> :crypto.hash(:md5, File.read!("priv/static/css/app.css")) |> Base.encode16
"1C16A9E2D001DDFF3292FC7C4EE9F89E"

This became an issue for me when I was replacing Brunch with Webpack. In my case, I configured Webpack to generate separate css and js. Each time Webpack runs, all of the css and js files are regenerated (even if there's no content change) which causes file mtime changes. So, if all I was doing was changing some css I would get a full page reload in my phoenix app since changes were detected in the js files.

I suspect most Webpack users will opt to use webpack-dev-server for HMR. I have already made that change. However, this could present an integration issue with other build tools depending on how they work. For Brunch, it doesn't seem to be an issue because it doesn't touch the js files when the css files are built.

@chrismccord
Copy link
Member

Since you are using HMR, can you simply remove the js pattern from your live reload config? Or do you still require full phoenix_live_reload page refresh for js changes. I would prefer to wait on this change since it's the first time this has come up and I would like to avoid hitting disk if possible.

@skinandbones
Copy link
Author

skinandbones commented Aug 1, 2017

Hi @chrismccord 👋 Thanks for the awesome work!

This can wait – it's not blocking me at all. I've gotten everything working the way I like by using webpack-dev-server, HMR, and css-hot-loader. It's a non-trivial setup to say the least and took some integration work in the phoenix app for :dev vs. :prod. Not as clean as the brunch setup.

My initial goal was to use webpack in the simplest configuration (no HMR) and rely on phoenix_live_reload (a build agnostic tool). Once I figured out what was going on with the false positive JS asset changes, I realized I would have to get the webpack-dev-server involved .... and all the complexity that goes with it.

If not for this issue, I honestly think we could write a phoenix + webpack integration guide (or even a generator) that wouldn't blow up complexity. Things could pretty much work as they do in the default generated app. I know a lot of people coming to Phoenix would be interested in that.

I agree, this will touch the filesystem and it would have to keep state.

@skinandbones
Copy link
Author

Since you are using HMR, can you simply remove the js pattern from your live reload config? Or do you still require full phoenix_live_reload page refresh for js changes.

With HMR (w/ webpack-dev-server), phoenix_live_reload will handle the refresh when the views and templates change. phoenix_live_reload isn't needed for refresh on js changes. In fact, the static assets generated by webpack don't even get written into the output directory so there are no file changes for phoenix_live_reload to detect.

@jameskerr
Copy link

I just ran into this as well. I loved brunch but it looks like it's not being maintained to the level I'm comfortable with. I switched from brunch to webpack.

My webpack setup outputs two files (js/index.js, css/app.css) each time css or js changes. This causes the whole page to refresh when I just change a style.

It's a non-trivial setup to say the least and took some integration work in the phoenix app for :dev vs. :prod. Not as clean as the brunch setup.

@skinandbones, like you, I'm hesitant to dive into the complexity of HMR with the webpack-dev-server. I'd much rather use standard live reload. Are there any new solutions now? A year later?

@josevalim
Copy link
Member

@jameskerr did you take a look at how Phoenix master handles webpack? Phoenix v1.4 will ship with webpack, so maybe this issue is fixed there (or maybe now all applications will have this issue).

@josevalim
Copy link
Member

Actually, it seems Phoenix master has this issue.

@chrismccord should we move this to the Phoenix repo so we can track it for the release?

@jameskerr
Copy link

Thanks for the quick response @josevalim . I just took a look at Phoenix master and found code very similar to what I just setup today. That's encouraging. I'll be tracking this. Please point me in any direction I might be able to help.

@josevalim
Copy link
Member

@jameskerr if you want to give this a try, it would be awesome. I think caching the MD5s on the Phoenix channel as proposed by @skinandbones is a good starting point.

@jameskerr
Copy link

I'll take look!

@chrismccord
Copy link
Member

caching at the channel side won't be enough because the page refreshes and discards all channel state before starting a new channel. We can however have a separate process that boots with the app which watches for changes and the channel subscribes to it.

@jeregrine
Copy link
Member

Should/Could we ship webpack with HMR/CSS loading using webpack's tools and reserve the phoenix loader for template/view changes?

@josevalim
Copy link
Member

@jeregrine I don't want to add more stuff for us to debug, another server to run, etc.

@skinandbones
Copy link
Author

@jeregrine Agree with @josevalim. I would not recommend using webpack for CSS hot reloading by default. I've been using a webpack HMR setup in my Phoenix apps since I submitted this issue, but it's not for everyone. The setup ripples into many things and stuff gets weird if you don't understand what's going on.

@nkezhaya
Copy link

Super old thread, but just wanted to show how this could be done with esbuild. This is assets/build.js:

const fs = require("fs-extra")
const glob = require("glob")
const crypto = require("crypto")
const esbuild = require("esbuild")
const postCssPlugin = require("esbuild-plugin-postcss2")

const mode = process.env.NODE_ENV === "production" ? "production" : "development"
const devMode = mode === "development"

const sourcemap = devMode ? "external" : false
const minify = !devMode
const commonOptions = { sourcemap, minify, write: false }

// Keep track of file hashes so we only write the ones that changed
const getDigest = string => {
  const hash = crypto.createHash("sha1")
  const data = hash.update(string, "utf-8")
  const digest = data.digest("hex")

  return digest
}

const getFileDigest = path => {
  const exists = fs.pathExistsSync(path)

  if (!exists) {
    return null
  }

  if (fs.statSync(path).isDirectory()) {
    return null
  }

  return getDigest(fs.readFileSync(path))
}

const writeResult = result => {
  for (let { path, contents } of result.outputFiles) {
    const digest = getDigest(contents)

    if (getFileDigest(path) !== digest) {
      fs.outputFile(path, contents)
    }
  }

  return result
}

// Main app

esbuild.build({
  entryPoints: ["js/App.js"],
  bundle: true,
  target: "es2016",
  outfile: "../priv/static/js/app.js",
  ...commonOptions
}).then(writeResult)

esbuild.build({
  entryPoints: ["css/app.css"],
  bundle: true,
  outfile: "../priv/static/css/app.css",
  external: ["*.woff", "*.gif"],
  plugins: [postCssPlugin.default({
    plugins: [
      require("postcss-preset-env"),
      require("postcss-import"),
      require("tailwindcss"),
      require("postcss-mixins"),
      require("postcss-nested")
    ]
  })],
  ...commonOptions
}).then(writeResult)

// Copy assets/static to priv/static

function filter(src, dest) {
  const exists = fs.pathExistsSync(dest)

  if (!exists) {
    return true
  }

  if (fs.statSync(dest).isDirectory()) {
    return true
  }

  return getFileDigest(src) !== getFileDigest(dest)
}

fs.copySync("./static", "../priv/static", { filter })

@nickjj
Copy link

nickjj commented Dec 29, 2021

It would be very useful if this was included by default. I'm also using esbuild with a much more simplified copy file approach. Changing 1 line of JS results in every static file being live reloaded when their contents didn't change.

You end up with this type of log output when editing any JavaScript:

js_1        | [watch] build finished
web_1       | [debug] Live reload: priv/static/android-chrome-512x512.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/apple-touch-icon.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/images/phoenix.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/mstile-150x150.png
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg
web_1       | [debug] Live reload: priv/static/safari-pinned-tab.svg

As a workaround whitepaperclip's solution works with Node 16.x. You could also avoid the 3rd party fs-extra Node dependency and replace fs.pathExistsSync with fs.existsSync to use Node's standard library.

If esbuild is being pushed as the Phoenix default it would be quite nice to have this behavior handled at the Phoenix Live Reload level, especially if you're pushing a message of not needing Node. You need Node to use a custom esbuild config.

@nkezhaya
Copy link

fs.pathExistsSync still works, but note the const fs = require("fs-extra"); at the top.

@nickjj
Copy link

nickjj commented Dec 29, 2021

fs.pathExistsSync still works, but note the const fs = require("fs-extra"); at the top.

Ah good catch. fs.existsSync works with Node's standard library. fs-extra looks to be a 3rd party package.

@Gazler Gazler closed this as completed Sep 16, 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 a pull request may close this issue.

8 participants