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

Record the compilation digest even on webpack error #2117

Merged
merged 2 commits into from Jun 10, 2019

Conversation

Taytay
Copy link
Contributor

@Taytay Taytay commented Jun 3, 2019

Fixes #2113

Previously, in #1764, the recording of the compilation digest was ensured to happen only after webpacker ran, but during that fix, it was also made that the digest was only recorded if Webpack succeeded. However, whether Webpack succeeds is not a measure of whether or not webpack emits output, meaning that the digest wouldn't be recorded in the case of a Typescript compilation error, even though new output file(s) were emitted. This divergence could cause either too many or too few compilations, as noted in #2113. This fix still moves the writing of the digest after Webpack runs, but does so regardless of whether Webpack succeeds.

@jakeNiemiec jakeNiemiec added the WIP This PR is work in progress label Jun 3, 2019
@jakeNiemiec
Copy link
Member

@frodsan It has been awhile, but can you check to see if this PR works in tangent with the test you describe here #1764 (comment)

If I don't hear back in a few days, I can make a rough approximation.

@jakeNiemiec
Copy link
Member

We also need to address the test for failing builds: https://github.com/rails/webpacker/blob/master/test/compiler_test.rb#L52

@Taytay If you want me to take care of this, you can allow me to edit with:
image

@Taytay
Copy link
Contributor Author

Taytay commented Jun 4, 2019

Doh! I totally missed that test. My apologies! I've got "allow edits" checked, so I would indeed appreciate it if you could add the fix!

If it returns stale, the error message will never make it to the `webpack-dev-server` overlay. This also fixes typescript related problems in rails#2113
@jakeNiemiec jakeNiemiec added bug and removed WIP This PR is work in progress labels Jun 7, 2019
Copy link
Member

@jakeNiemiec jakeNiemiec left a comment

Choose a reason for hiding this comment

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

Looks good. This should enable some missing webpack-dev-server features. Thanks for your contribution.

@gauravtiwari gauravtiwari merged commit 195396f into rails:master Jun 10, 2019
@justin808
Copy link
Contributor

@gauravtiwari @jakeNiemiec Hi! I think this should be changed to check that at least the manifest.json was written.

If there's some error on the webpack build, the cache digest is still saved. Then you go to update your webpack config and it's confusing that there is no recompile.

If somebody wants TS suggestions, that can be done by editor configuration or other things separate from the webpack build process.

I can't think of a valid case where the digest should get written if the output of webpack is non-zero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants