feat: deprecate setup in favor of before and after hooks #1108

mrtnbroder · 2017-09-20T07:26:58Z

What kind of change does this PR introduce?

feature

Did you add or update the examples/?
yes, examples/node-api-middleware

Summary

fixes #869

Does this PR introduce a breaking change?

Not a breaking change, but a deprecation.

Other information
see #869

jsf-clabot · 2017-09-20T07:26:59Z

All committers have signed the CLA.

shellscape

looks like there's some test failures to address as well

shellscape · 2017-09-20T22:42:33Z

lib/Server.js

@@ -323,7 +323,9 @@ function Server(compiler, options) {

    middleware: () => {
      // include our middleware to ensure it is able to handle '/index.html' request after redirect
+      if (typeof options.before === 'function') { options.before(app, this); }


preferably we'd follow the same pattern established on line 342 with the setup middleware. 'before' would be inserted into that array before setup and after would be added to the array before line 355

Fixed!
I've changed the example for middleware as well, saw that one was using the setup hook which is now the before hook.

codecov · 2017-09-21T08:01:25Z

Codecov Report

Merging #1108 into master will increase coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master   #1108      +/-   ##
=========================================
+ Coverage   71.92%     72%   +0.08%     
=========================================
  Files           5       5              
  Lines         463     468       +5     
  Branches      148     151       +3     
=========================================
+ Hits          333     337       +4     
- Misses        130     131       +1

Impacted Files	Coverage Δ
lib/Server.js	`78.61% <83.33%> (+0.02%)`	⬆️

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 8bc6daa...40c6ece. Read the comment docs.

shellscape · 2017-09-25T15:11:32Z

lib/Server.js

      if (typeof options.setup === 'function') { options.setup(app, this); }
    }
  };

-  const defaultFeatures = ['setup', 'headers', 'middleware'];
-  if (options.proxy) { defaultFeatures.push('proxy', 'middleware'); }
+  const defaultFeatures = ['before', 'setup', 'headers', 'middleware', 'after'];


after shouldn't be here. same goes for the changes on lines 353, 357. rather, it should be pushed to defaultFeatures before line 365 - just before the forEach

fixes webpack#869

bdwain · 2017-09-27T15:46:04Z

lib/Server.js

      if (typeof options.setup === 'function') { options.setup(app, this); }
    }
  };

-  const defaultFeatures = ['setup', 'headers', 'middleware'];
+  const defaultFeatures = ['before', 'setup', 'headers', 'middleware'];


@shellscape @mrtnbroder doesn't setup being in this array mean that everyone will see the deprecation warning no matter what? Shouldn't it not be in there but one of the things below this say if(options.setup) defaultFeatures.push('setup') or something? (or maybe keep it in but if options.setup is absent then remove it).

It seems that the loop on line 367 will always run the deprecation warning.

I'm asking because when i upgraded to 2.9 i noticed the deprecation warning even though I don't use this hook at all (though it may just be that a dependency of mine needs upgrading. I'm not totally sure)

either that or putting the logs on lines 346-347 inside the if on line 348

I made #1120 for this

cletusw · 2017-09-27T23:19:31Z

It took me a while to track down where this error was coming from. Please add a link to the changelog in the error message (or at least include the text webpack-dev-server somewhere).

shellscape · 2017-09-27T23:23:21Z

@cletusw double posting the same comment in multiple places is generally frowned upon, please avoid that in the future (referencing your copied comment on 8de5d0a). logging in the module will be getting an overhaul in v3. the message being shown isn't an error message; it's a vanilla log message. Deprecation warnings are never error messages.

cletusw · 2017-09-27T23:24:07Z

@shellscape Sorry, I didn't realize that you were also on this issue. Sorry for the double post.

cletusw · 2017-09-27T23:26:20Z

As for error vs. log, I guess my original message wasn't entirely clear. What I meant was, I saw the log text, but I had no idea how to fix it since it wasn't at all clear which setup it was referring to. My mocha test files? Some random setup method in my code? A third-party library? My ask was that the deprecation message at least include the text webpack-dev-server so I know where the message is coming from. Ideally, it would include a link to a page somewhere on the web explaining what the deprecation entails. Sorry I wasn't clear originally.

shellscape · 2017-09-27T23:27:01Z

logging in the module will be getting an overhaul in v3.

cletusw · 2017-09-27T23:28:40Z

logging in the module will be getting an overhaul in v3.

Not sure what you mean. Are you saying the deprecation warning will be ambiguous until v3 at which time it will change to say webpack-dev-server: "setup" is deprecated...?

bdwain · 2017-09-27T23:37:16Z

agreed about the log prefix. most log messages I've seen from webpack dev server start with [WDS]. I spent about 20 minutes this morning googling and searching to see where the warning came from because it's just a random error in the console.

It seems like a simple enough change that it shouldn't have to wait until V3's log overhaul. People are going to start seeing the deprecation warning soon and have no idea where it's coming from. @shellscape Would a PR to add [WDS] be approved?

shellscape · 2017-09-27T23:47:48Z

@bdwain very much appreciate the initiative, but we're about to announce a new feature freeze to focus on v3.0.0, and logging throughout the entire module needs improvement as it has the same/similar shortcomings. you can follow progress on v3 here. I won't comment yet on what that will entail because we haven't had time to vet all of the available options. in the meantime we ask for patience.

cletusw · 2017-09-28T00:01:53Z

I would suggest removing this message then. As it is it is unactionable and will generate lots of confusion.

…

On Wed, Sep 27, 2017, 5:47 PM Andrew Powell ***@***.***> wrote: @bdwain <https://github.com/bdwain> very much appreciate the initiative, but we're about to announce a new feature freeze to focus on v3.0.0, and logging throughout the entire module needs improvement as it has the same/similar shortcomings. you can follow progress on v3 here <https://github.com/webpack/webpack-dev-server/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22Version+3%22>. I won't comment yet on what that will entail because we haven't had time to vet all of the available options. in the meantime we ask for patience. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1108 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0prrc7xRfusLcDp3jN6o26N8_E0NDLks5smt6ngaJpZM4Pdcue> .

shellscape · 2017-09-28T00:08:05Z

@cletusw I think you've made your position abundantly clear 😄 The change will stand as-is for the moment. I'll likely abstain from commenting on this PR moving forward, but please feel free to continue the discussion.

`setup` was deprecated in webpack/webpack-dev-server#1108

shellscape suggested changes Sep 20, 2017

View reviewed changes

mrtnbroder force-pushed the mb/feature/middleware-before-after branch 2 times, most recently from fd842ed to 89ae007 Compare September 21, 2017 07:58

mrtnbroder force-pushed the mb/feature/middleware-before-after branch from 89ae007 to 546adaa Compare September 21, 2017 08:02

shellscape suggested changes Sep 25, 2017

View reviewed changes

feat: deprecate setup in favor of before and after hooks

febf76f

fixes webpack#869

mrtnbroder force-pushed the mb/feature/middleware-before-after branch from 546adaa to febf76f Compare September 26, 2017 07:27

Merge branch 'master' into mb/feature/middleware-before-after

40c6ece

shellscape approved these changes Sep 26, 2017

View reviewed changes

shellscape merged commit dcb4e3d into webpack:master Sep 26, 2017

mrtnbroder deleted the mb/feature/middleware-before-after branch September 26, 2017 20:16

bdwain reviewed Sep 27, 2017

View reviewed changes

bdwain mentioned this pull request Sep 27, 2017

Deprecation Warning for Setup Always Appears #1120

Closed

3 tasks

xtian added a commit to xtian/create-elm-app that referenced this pull request Feb 6, 2018

config/webpackDevServer.config.js: Fix deprecation warning

5df599e

`setup` was deprecated in webpack/webpack-dev-server#1108

xtian mentioned this pull request Feb 6, 2018

config/webpackDevServer.config.js: Fix deprecation warning halfzebra/create-elm-app#230

Merged

renovate bot mentioned this pull request Mar 21, 2018

Update webpack-dev-server to 3.1.4-abandoned sbardian/DebtTrackerAPI#22

Closed

GerHobbelt mentioned this pull request Jan 9, 2023

[Snyk] Fix for 1 vulnerabilities GerHobbelt/webpack-dev-server#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deprecate setup in favor of before and after hooks #1108

feat: deprecate setup in favor of before and after hooks #1108

mrtnbroder commented Sep 20, 2017 •

edited

jsf-clabot commented Sep 20, 2017 •

edited

shellscape left a comment

shellscape Sep 20, 2017

mrtnbroder Sep 21, 2017

codecov bot commented Sep 21, 2017 •

edited

shellscape Sep 25, 2017

mrtnbroder Sep 26, 2017

bdwain Sep 27, 2017

bdwain Sep 27, 2017

bdwain Sep 27, 2017

cletusw commented Sep 27, 2017

shellscape commented Sep 27, 2017 •

edited

cletusw commented Sep 27, 2017

cletusw commented Sep 27, 2017

shellscape commented Sep 27, 2017

cletusw commented Sep 27, 2017

bdwain commented Sep 27, 2017

shellscape commented Sep 27, 2017

cletusw commented Sep 28, 2017 via email

shellscape commented Sep 28, 2017

Navigation Menu

feat: deprecate setup in favor of before and after hooks #1108

feat: deprecate setup in favor of before and after hooks #1108

Conversation

mrtnbroder commented Sep 20, 2017 • edited

jsf-clabot commented Sep 20, 2017 • edited

shellscape left a comment

Choose a reason for hiding this comment

shellscape Sep 20, 2017

Choose a reason for hiding this comment

mrtnbroder Sep 21, 2017

Choose a reason for hiding this comment

codecov bot commented Sep 21, 2017 • edited

Codecov Report

shellscape Sep 25, 2017

Choose a reason for hiding this comment

mrtnbroder Sep 26, 2017

Choose a reason for hiding this comment

bdwain Sep 27, 2017

Choose a reason for hiding this comment

bdwain Sep 27, 2017

Choose a reason for hiding this comment

bdwain Sep 27, 2017

Choose a reason for hiding this comment

cletusw commented Sep 27, 2017

shellscape commented Sep 27, 2017 • edited

cletusw commented Sep 27, 2017

cletusw commented Sep 27, 2017

shellscape commented Sep 27, 2017

cletusw commented Sep 27, 2017

bdwain commented Sep 27, 2017

shellscape commented Sep 27, 2017

cletusw commented Sep 28, 2017 via email

shellscape commented Sep 28, 2017

mrtnbroder commented Sep 20, 2017 •

edited

jsf-clabot commented Sep 20, 2017 •

edited

codecov bot commented Sep 21, 2017 •

edited

shellscape commented Sep 27, 2017 •

edited