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

fix: infinity refresh on warnings #4006

Merged
merged 5 commits into from Nov 4, 2021
Merged

fix: infinity refresh on warnings #4006

merged 5 commits into from Nov 4, 2021

Conversation

alexander-akait
Copy link
Member

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes, WIP

Motivation / Use-Case

fixes #4005

Breaking Changes

No

Additional Info

No

@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #4006 (bf4cc45) into master (171dba9) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4006      +/-   ##
==========================================
- Coverage   92.60%   92.59%   -0.01%     
==========================================
  Files          14       14              
  Lines        1406     1405       -1     
  Branches      516      516              
==========================================
- Hits         1302     1301       -1     
  Misses         96       96              
  Partials        8        8              
Impacted Files Coverage Δ
client-src/index.js 77.98% <ø> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 171dba9...bf4cc45. Read the comment docs.

@alexander-akait alexander-akait merged commit 10da223 into master Nov 4, 2021
@alexander-akait alexander-akait deleted the issue-4005 branch November 4, 2021 18:46
@obsius
Copy link

obsius commented Nov 15, 2021

@alexander-akait Were the ramifications of this change intentional? I upgraded an old deployment recently and this change was counter to the old behavior and also my expectation that warnings would not prevent an HMR update or full page reload.

For example, the log message for an error states:
Errors while compiling. Reload prevented.
this is not the case for warnings:
Warnings while compiling.

Was the intent to only prevent this reload when the overlay is used?

    if (needShowOverlayForWarnings) {
      show("warning", _warnings);
    } else {
      reloadApp(options, status);
    }

@alexander-akait
Copy link
Member Author

Yes, expected, we should not do refresh on warnings, please provide reproducible example and I will show how to fix

@obsius
Copy link

obsius commented Nov 15, 2021

@alexander-akait Ok, but what I'm saying is that this behavior (calling reloadApp on warnings) has been around for a long time, at least 3 years:

image

I think this change should be communicated somehow, or perhaps a configuration setting to allow the old behavior made accessible.

@alexander-akait
Copy link
Member Author

alexander-akait commented Nov 15, 2021

@obsius Can you provide example of the problem? We should not add new options on every problem, if it is bug, we should fix it, there are a lot of changes over the past three years, as you can see keeping that, potentially create infinity reloads, and we can't just revert it

@obsius
Copy link

obsius commented Nov 16, 2021

@alexander-akait This PR does not contain a bug, but does present a significant change to how the webpack-dev-server client works. Anyone expecting HMR or page reloading after a compilation with warnings (the old behavior) may think their configuration is wrong or that some other plugin is in error.

I haven't looked at this repo before today, but from the commit history it seems that the old behavior was stable for a very long time, so it would make sense to let developers know of this sudden change.

For example, the log message for an error states:
Errors while compiling. Reload prevented.

Now that a compilation with warnings does not reload, the warning log message could be updated to this:
Warnings while compiling. Reload prevented.

This new behavior could also be mentioned in the release notes.

An example of this issue would be:

Developer has turned off the overlay for warnings.

Before this change:

  • webpack-dev-server sends message { type: "warnings", ... }
  • webpack-dev-server client prints warnings to console
  • webpack-dev-server client calls reloadApp
  • developer sees the page update

After this change:

  • webpack-dev-server sends message { type: "warnings", ... }
  • webpack-dev-server client prints warnings to console
  • webpack-dev-server client does not call reloadApp
  • developer wonders why the page is not updating

@alexander-akait
Copy link
Member Author

@obsius Again, please create reproducible test repo, sending warnings should not prevent reloading, if hash was changed, we should do reload

@alexander-akait
Copy link
Member Author

If you send warnings without hash changes, it means nothing was changed in compilation, so no reasons to reload page

@obsius
Copy link

obsius commented Nov 16, 2021

@alexander-akait Since you agree that a reload is desired after compilation with warnings, perhaps you can help me understand how a hash change propagates into a reload in the client?

The listener in the client for hash does not seem to call reloadApp:

  hash(hash) {
    status.previousHash = status.currentHash;
    status.currentHash = hash;
  },

The only place where reloadApp is called in version 4.5.0 is in the ok handler, but the ok message type is not sent from the server when a compilation produces warnings. In the previous version, reloadApp was called from both ok and warnings.

@alexander-akait
Copy link
Member Author

alexander-akait commented Nov 16, 2021

dev server connected with compilation using hooks and send ws message from server to client with hash

@obsius
Copy link

obsius commented Nov 16, 2021

@alexander-akait Here is the server log for a compilation that produces warnings:

image

(1) invalid is sent when compilation begins
(2) hash is sent after compilation (something has changed)
(3) warnings is sent after hash

What I am trying to explain to you, but for some reason is being lost in communication, is that the client is not updating after warnings anymore. I don't see any mechanism in the client code to respond solely to the hash message, so what I am asking you to help me understand is how reloadApp gets called in the client after a compilation that produced warnings. It no longer seems possible with this latest change.

@alexander-akait
Copy link
Member Author

Please create reproducible example, if you need help ASAP, it is the best choice

@obsius
Copy link

obsius commented Nov 16, 2021

@alexander-akait I don't need help ASAP, I'm trying to explain that this PR prevents HMR and page reloads after compilations with warnings. You're not taking my comments seriously. I'm asking a very straightforward question to you and you are being obstinate. I cannot make this issue any clearer to you at this point. You removed the only place besides the ok handler where reloadApp is called. The server does not send an ok message after a compilation with warnings, so HMR and page reloads no longer happen in this scenario.

@alexander-akait
Copy link
Member Author

@obsius Open an original issue and look at infinity loop, it was fixed here, we send hash on each compilation, no hash changes = no reloading, no blockers on client.

And stop blaming me, I'm trying to figure out what the problem is and request additional information, but you ignore my request. I am a developer like you, and I spend my free time on this. I believe that we may have a problem, I do not argue and do not claim that you are wrong, all I ask is to show an example.

If you have idea how to fix - PR welcome

@obsius
Copy link

obsius commented Nov 16, 2021

@alexander-akait Don't mean to blame you, and appreciate the work you are doing on this. I'm unfamiliar with this project and don't know what changes are desired, which is why I thought this was an undocumented change at first. I cross referenced an issue that may be related and has a repo linked.

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.

Compilation failure causes infinite refresh
2 participants