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(index): don't modifiy the default behavior for unhandledRejection #340

Merged
merged 1 commit into from
Sep 24, 2018

Conversation

HaNdTriX
Copy link
Contributor

@HaNdTriX HaNdTriX commented Sep 23, 2018

loud-rejection modifies the global promise/async rejection behaviour so uncaught rejections will be swallowed. This PR removes loud-rejection.

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

TBD

Summary

See: #339

Does this PR introduce a breaking change?

This PR introduces a breaking change since uncaught promises might end the process now.

Other information

Closes #339

This modules modifies the global promise/async rejection behaviour so no uncaught rejections will be swallowed.
@jsf-clabot
Copy link

jsf-clabot commented Sep 23, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 23, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #340      +/-   ##
=========================================
- Coverage   96.81%   96.8%   -0.02%     
=========================================
  Files           7       7              
  Lines         251     250       -1     
=========================================
- Hits          243     242       -1     
  Misses          8       8
Impacted Files Coverage Δ
index.js 97.43% <ø> (-0.07%) ⬇️

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 d75802b...0e22849. 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.

Looks good, i am afraid what it is breaking change.

@SpaceK33z SpaceK33z merged commit f0a8e3e into webpack:master Sep 24, 2018
@SpaceK33z
Copy link
Member

SpaceK33z commented Sep 24, 2018

Thanks @HaNdTriX!

@evilebottnawi I can't come up with a good reasoning as to why this would be a breaking change. I see this only as a bug, especially since in loud-rejection's README it is mentioned it should not be used in packages. I think fixing this is in a minor release rather than waiting for a major release (which can take months) is better for more users.

@alexander-akait
Copy link
Member

alexander-akait commented Sep 24, 2018

@SpaceK33z Agree, i think we should do release on after this merge (already), but no changelog or infromation https://github.com/webpack/webpack-dev-middleware/releases/tag/v3.4.0, need fix

@SpaceK33z
Copy link
Member

@evilebottnawi Fixed already, was writing the changelog by hand because I saw the new npm run release script too late (wasn't there last time I published a while ago 😄)

@HaNdTriX
Copy link
Contributor Author

Wow that was fast. Thanks a lot 🙏!

@michael-ciniawsky
Copy link
Member

Why was this released as a minor version bump it should either have been a patch (v3.3.2) or a major (v4.0.0)? The CHANGELOG is also compelete missing https://github.com/webpack/webpack-dev-middleware/blob/master/CHANGELOG.md as it seems (I fix it manually this time, please don't do anything). Could you guys please just leave the PR(s) open if you are not familiar with the release 'process' (yet) (I merge approved PR's and release everything once per day normally), or ask if in doubt? Everytime someone else cuts a release in one of webpack/webpack-contrib repos it is more or less done wrong

@SpaceK33z
Copy link
Member

@michael-ciniawsky this was a minor version bump because I found it too tricky to publish as patch, yet unnecessary to release as a major. Maybe in hind sight not the best option, but too late.

Anyway, as I said above here already I didn't know there was a npm run release script. I've been maintaining this repository for two years now, and suddenly there is a release script I only could've known if I super carefully read every line of the package.json. If you want to prevent people from doing it wrong, please block using npm version directly. I'm sure there are ways to do that. Without that, this mistake will be repeated and repeated by me and others.

@michael-ciniawsky michael-ciniawsky removed this from the 3.4.1 milestone Sep 24, 2018
@michael-ciniawsky michael-ciniawsky changed the title refactor(index): remove loud-rejection fix(index): don't modifiy the default behaviour for unhandledRejections Sep 24, 2018
@michael-ciniawsky michael-ciniawsky changed the title fix(index): don't modifiy the default behaviour for unhandledRejections fix(index): don't modifiy the default behaviour for unhandledRejection Sep 24, 2018
@michael-ciniawsky michael-ciniawsky changed the title fix(index): don't modifiy the default behaviour for unhandledRejection fix(index): don't modifiy the default behavior for unhandledRejection Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swallows all async errors in server context
5 participants