Skip to content

Conversation

eqyiel
Copy link

@eqyiel eqyiel commented Dec 25, 2018

  • 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?

N/A

Motivation / Use-Case

The code that is removed here added a beforeunload event check in order to prevent the reload from happening if the page is already refreshing.

However, it is not always the case that after beforeunload page will continue reloading, for two reasons at least:

  • beforeunload is cancelable, by spec
  • custom protocol use case

In my case, there is an API that writes to window.location using a custom protocol. window.location changes but the JS context is preserved, leaving webpack-dev-server incapacitated.

This PR fixes that (and should also apply cleanly to the v2 branch).

Ref: #841
Ref: #544

Breaking Changes

N/A

Additional Info

N/A

@jsf-clabot
Copy link

jsf-clabot commented Dec 25, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 25, 2018

Codecov Report

Merging #1611 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1611      +/-   ##
==========================================
- Coverage   74.07%   73.93%   -0.15%     
==========================================
  Files          10       10              
  Lines         679      679              
==========================================
- Hits          503      502       -1     
- Misses        176      177       +1
Impacted Files Coverage Δ
lib/Server.js 81.01% <0%> (-0.27%) ⬇️

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 4b7a828...4d94294. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

We need search way to write tests on this, good work, but it is breaking change and should be merge to next major release

@alexander-akait
Copy link
Member

alexander-akait commented Dec 25, 2018

Also please accept CLA (bug on CLA side)

@alexander-akait
Copy link
Member

No need right now, thanks

@eqyiel
Copy link
Author

eqyiel commented Feb 12, 2019

@evilebottnawi I'm confused about the change from

We need search way to write tests on this, good work, but it is breaking change and should be merge to next major release

to

No need right now, thanks

If there's something that needs improvement, please tell me about it!

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.

3 participants