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

Skip contenthash when not production #3060

Merged

Conversation

justin808
Copy link
Contributor

Per the webpack docs on better performance, the contenthash should be
skipped when not on production.

https://webpack.js.org/guides/build-performance/#avoid-production-specific-tooling

@thomasklemm
Copy link

Hey @justin808, this PR looks like it reverts the changes made in #2954, which implemented a fix for stylesheets not being reloaded in certain browsers in development. Would the issue described in that PR return if these changes were merged? Code-wise it seems to revert settings for the MiniCssExtractPlugin to what they were before #2954 was merged if I see correctly?

})
)
}

return plugins
}

// Don't use contentHash except for production for performance
// https://webpack.js.org/guides/build-performance/#avoid-production-specific-tooling
const filenameHash = isProduction ? '-[contenthash]' : '';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const filenameHash = isProduction ? '-[contenthash]' : '';
const hash = isProduction ? '-[contenthash]' : '';

For consistency

Copy link
Member

Choose a reason for hiding this comment

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

We are not using semicolons at all. So should strip instead.

@guillaumebriday guillaumebriday requested a review from dhh July 6, 2021 13:55
@justin808
Copy link
Contributor Author

Hey @justin808, this PR looks like it reverts the changes made in #2954, which implemented a fix for stylesheets not being reloaded in certain browsers in development. Would the issue described in that PR return if these changes were merged? Code-wise it seems to revert settings for the MiniCssExtractPlugin to what they were before #2954 was merged if I see correctly?

Per
https://github.com/rails/webpacker/pull/3060/files#diff-7af8667a3e36201db57c02b68dd8651883d7bfc00dc9653661be11cd31feeccdR71-R72

CSS gets the hash, not contenthash.

Per the webpack docs on better performance, the contenthash should be
skipped when not on production.

https://webpack.js.org/guides/build-performance/#avoid-production-specific-tooling
@justin808 justin808 force-pushed the justin808/skip-conenthash-when-not-production branch from e2309f0 to 24a01d8 Compare July 7, 2021 08:31
@justin808
Copy link
Contributor Author

@guillaumebriday Comments addressed. Tomorrow, HST, I'll try to update https://github.com/shakacode/react_on_rails_tutorial_with_ssr_and_hmr_fast_refresh with this change to test this out.

@justin808 justin808 force-pushed the justin808/skip-conenthash-when-not-production branch from 24a01d8 to 96062a7 Compare July 7, 2021 08:41
@dhh dhh merged commit 5dae8b5 into rails:master Jul 7, 2021
@justin808 justin808 deleted the justin808/skip-conenthash-when-not-production branch July 8, 2021 07:26
@t27duck
Copy link
Contributor

t27duck commented Jul 15, 2021

I'm slightly perplexed as to what the end result is going to be now. For those of use using webpacker for straight up JS and CSS use (no react), does this plus HMR allow in dev mode to let us change JS and CSS and have those changes be shown in the browser?

If the filenames rename the same, browsers will treat it as a cached file and not attempt to pull in the new code. That would force developers to run their browsers with caching disabled via the network tab in the inspector.

That was the whole point of adding the hashes in #2954 for CSS as the JS code was being refreshed on save but not the CSS.

@t27duck
Copy link
Contributor

t27duck commented Jul 15, 2021

A more specific example:

I have apps thare are simply turbo, stimulus, and tailwind all going through the current webpacker beta. No react. No SSR. Just straight up bundled JS and CSS. I start up bin/webpacker-dev-server and start editing my JS and CSS. On save, the page refreshes and my changes show up because the CSS and JS filenames change. With this change, it looks to me like they will continue to alwyas be application.js and application.css which would cause the browser to not pull the latest files by default.

@justin808
Copy link
Contributor Author

I'm pretty sure that the Rails server lets the browser know to not cache the files during development.

@t27duck
Copy link
Contributor

t27duck commented Jul 15, 2021

Appolgies if I came off strong earlier. I managed (barely) to get master hooked up to an app and did confirm that these changes (plus #3031) does appear to properly refresh any changed CSS and JS (at least with some limited local testing).

The whole webpack/JS ecosystem is still a bit of a black box to me, but somehow this is pushing changes without having to have file names with different hashes in them. 🤷

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.

5 participants