BREAKING CHANGES: create static option and remove replaced options #2670

knagaitsev · 2020-07-04T16:53:52Z

For Bugs and Features; did you add new tests?

Not yet

Motivation / Use-Case

static option is replacing many existing options. See options.json.

Breaking Changes

Remove contentBase, contentBasePublicPath, serveIndex, staticOptions, watchContentBase, watchOptions in favor of static
API changes related to contentBase

Additional Info

codecov · 2020-07-04T17:02:42Z

Codecov Report

Merging #2670 into v4 will increase coverage by 0.02%.
The diff coverage is 98.57%.

@@            Coverage Diff             @@
##               v4    #2670      +/-   ##
==========================================
+ Coverage   92.85%   92.88%   +0.02%     
==========================================
  Files          37       37              
  Lines        1316     1293      -23     
  Branches      354      344      -10     
==========================================
- Hits         1222     1201      -21     
+ Misses         89       88       -1     
+ Partials        5        4       -1

Impacted Files	Coverage Δ
lib/Server.js	`96.33% <96.87%> (+0.35%)`	⬆️
lib/utils/createConfig.js	`95.45% <100.00%> (+0.85%)`	⬆️
lib/utils/normalizeOptions.js	`100.00% <100.00%> (ø)`
lib/utils/status.js	`80.00% <0.00%> (-10.00%)`	⬇️

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 2bbd36f...30b3aca. Read the comment docs.

knagaitsev · 2020-07-05T21:23:43Z

@evilebottnawi Do we need directory to be a required property for the static object option? There is a chance the user could want to set other static options and keep directory as the default current working directory.

alexander-akait · 2020-07-06T13:09:24Z

lib/options.json

+                "minLength": 1
+              },
+              {
+                "type": "object"


Let's create definition fro object notation and use here it

Example:

"anyOf": [ { "type": "boolean" }, { "$ref": "#/definitions/StaticString" }, { "$ref": "#/definitions/StaticObject" }, { "type": "array", "items": { "anyOf": [ { "$ref": "#/definitions/StaticString" }, { "$ref": "#/definitions/StaticObject" } ] }, "minItems": 1 } ]

alexander-akait · 2020-07-06T13:14:45Z

lib/utils/normalizeOptions.js

+    ) {
+      options.static.watch = {};
+    }
+  }


I think like this:

const defaultOptionsForStatic = { directory: process.cwd(), staticOptions: {}, publicPath: ['/'], serveIndex: { icons: true }, // Respect options from compiler.watchOptions watch: { // ... }, }; if (options.static === 'undefined') { options.static = [{ defaultOptionsForStatic }]; } else if (typeof options.static === 'boolean') { options.static = options.static ? [defaultOptionsForStatic] : false; } else if (typeof options.static === 'string') { options.static = [ { ...defaultOptionsForStatic, directory: options.static }, ]; } else if (Array.isArray(options.static)) { options.static = options.static.map((item) => { if (typeof item === 'string') { return { ...defaultOptionsForStatic, directory: item }; } return { ...defaultOptionsForStatic, ...item }; }); } else { options.static = [{ ...defaultOptionsForStatic, ...options.static }]; }

Note - we should support --no-static for CLI

alexander-akait · 2020-07-07T10:29:46Z

bin/cli-flags.js

      type: Boolean,
-      describe: 'Enable live-reloading of the content-base.',
+      describe: 'Enable live-reloading of the static directory.',


I think we should not provide here a lot of stuff, --static=path/to/directory and --no-static are enough, for other complex examples you need to use the configuration

So we will get rid of the serve-index CLI flag?

Also, it seems our CLI system does not work well with a no-static flag, since static would have to be a string flag, not sure how to work around it without a hacky method

Edit: Never mind, I think I figured it out

alexander-akait · 2020-07-07T10:30:36Z

lib/Server.js

-    }
+
+      if (staticOption.watch) {
+        this._watch(staticOption.directory, staticOption.watch);


Let's rename _watch to watchFiles

alexander-akait · 2020-07-07T10:31:39Z

lib/utils/normalizeOptions.js

+  const watchOptions = watchOptionsConfig
+    ? watchOptionsConfig.watchOptions
+    : {};
+  console.log(compiler.options.watch);


console.log

knagaitsev · 2020-08-02T20:52:39Z

test/server/static-directory-option.test.js

-      }
-    });
-    it('Should throw exception (number)', (done) => {
+    it('Should throw exception (external url)', (done) => {


@evilebottnawi Is it correct that I should be throwing if isAbsoluteUrl for the static option is true?

No sure, I think we should improve it on schema-utils side, and support only paths, no URLs

knagaitsev · 2020-08-02T21:27:17Z

lib/options.json

+          ]
+        }
+      },
+      "required": ["directory"]


Do we need this to be required? It seems unnecessary and may be confusing if the user is just specifying watch options or something.

I'm going to remove this and let it default to process.cwd() as usual if not provided, unless you think it is needed

Yep, you are right, we don't need it, because you can set only watchOptions

alexander-akait · 2020-08-03T15:57:28Z

bin/options.js

@@ -75,16 +70,11 @@ const options = {
    group: SSL_GROUP,
    describe: 'HTTP/2, must be used with HTTPS',
  },
-  'static-directory': {
+  static: {


Can be multiple

@evilebottnawi Like --static=example/dir1,example/dir2?

Like --static=example/dir1 --static=example/dir2

knagaitsev · 2020-08-03T16:03:16Z

I'm getting System limit for number of file watchers reached error repeatedly if I do npm run test, though individual tests seem to be fine. Does it look like I'm doing anything wrong with opening or closing the watcher?

alexander-akait · 2020-08-03T16:12:35Z

We need to set https://github.com/paulmillr/chokidar/blob/master/index.js#L325 to true for tests, to avoid it

…ix https test

…le log

alexander-akait · 2020-08-04T10:12:50Z

lib/Server.js

-    if (this.options.watchContentBase) {
-      runnableFeatures.push('watchContentBase');
+    if (this.options.static) {
+      runnableFeatures.push('serveIndex', 'watchStatic');


I think we can union static/serveIndex/watchStatic in one method and static middleware, using one without the other doesn't make sense at all

@evilebottnawi I tried that but it causes breaking changes when historyApiFallback is enabled. It seems that the order of middleware things that are enabled surrounding historyApiFallback is important

https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L554-L575

Why do not put static feature after historyApiFallback, or maybe we should union historyApiFallback with static features?

I think it makes sense to add it to static since it deals with serving static HTML content

Yep, i think we should add the historyApiFallback option to static

@evilebottnawi After some searching I figured out that contentBaseFiles (

webpack-dev-server/lib/Server.js

Line 562 in 4ab1f21

runnableFeatures.push('contentBaseFiles');

) feature was placed before and after historyApiFallback for this reason:

https://github.com/bripkens/connect-history-api-fallback/blob/master/examples/static-files-and-index-rewrite/README.md#configuring-the-middleware

Is it possible that the middleware feature is added multiple times for a similar reason? Changing it so that there is only 1 middleware pushed does not seem to break any tests, but maybe tests were not made for it?

I think I will not mess with the middleware runnable feature, but will investigate it later and see if I can add some tests for having it get pushed multiple times.

I have made the 3 features: static, serveIndex, and watchStatic. It would be ideal to have only the static feature, but it is complicated by the historyApiFallback feature so having 3 features is the easiest solution at the moment

It is very weird, because if we can't find something in static and history we should return 404, not trying to do it again 😕 maybe I something don't understand

@evilebottnawi It isn't a matter of finding something or returning a 404, but rather the priority of static vs history when they are both trying to serve something from the same path. I still don't know exactly why it works, but placing static middleware before and after seems to ensure that static files take priority over the history api fallback file.

Yep, I see, very weird, but we can't do something here 😞

alexander-akait · 2020-08-17T12:29:45Z

/cc @hiroppy @Loonride But I am fine with merge it

/cc @Loonride Can we open new issue about history and static weird problem and resolve it in future

alexander-akait · 2020-08-17T16:16:12Z

lib/Server.js

        this.setupStaticFeature();
      },
-      // Todo rename to `serveIndex` in future major release
-      contentBaseIndex: () => {
+      serveIndex: () => {


staticServeIndex

and this.setupStaticServeIndexFeature

alexander-akait · 2020-08-17T16:16:24Z

lib/Server.js

        this.setupServeIndexFeature();
      },
-      // Todo rename to `watchStatic` in future major release
-      watchContentBase: () => {
+      watchStatic: () => {


staticWatch

and this.setupStaticWatchFeature

alexander-akait

To keep static name feature consistently

alexander-akait · 2020-08-17T16:22:45Z

lib/Server.js

@@ -382,10 +382,10 @@ class Server {
      static: () => {
        this.setupStaticFeature();
      },
-      serveIndex: () => {
+      staticServeIndex: () => {
        this.setupServeIndexFeature();


And let's rename method names: this.setupStaticServeIndexFeature, as I written above 🥇

And let's rename method names: this.setupStaticServeIndexFeature, as I written above

Just did that 👍

alexander-akait

/cc @hiroppy

Feel free to merge after CI will be green

knagaitsev requested review from alexander-akait and hiroppy as code owners July 4, 2020 16:53

alexander-akait reviewed Jul 6, 2020

View reviewed changes

alexander-akait reviewed Jul 7, 2020

View reviewed changes

knagaitsev force-pushed the chore/static-option branch from a4e11f0 to 5caa38b Compare July 7, 2020 15:59

knagaitsev mentioned this pull request Jul 13, 2020

chore: rename isLocalIp flag #2672

Merged

6 tasks

knagaitsev force-pushed the chore/static-option branch from 5caa38b to f5597e4 Compare July 23, 2020 05:17

knagaitsev added 8 commits August 2, 2020 10:53

chore(options): create static option and remove replaced options

972cd03

chore(options): remove contentBase option

2529175

chore(cli): rename flags to fit static option

64b767e

chore(options): add more default handling for static option

745fb81

chore(server): remove console log

88f08d2

chore(server): rename _watch to watchFiles

e365430

test(utils): add more tests and update for normalizeOptions

16e0221

test(utils): add more normalizeOptions tests

497f35d

knagaitsev force-pushed the chore/static-option branch from e3ef515 to 497f35d Compare August 2, 2020 15:56

test(options): update contentBase test to be static directory test

9fb688c

knagaitsev commented Aug 2, 2020

View reviewed changes

knagaitsev added 3 commits August 2, 2020 20:44

test(option): update contentBasePublicPath test to static publicPath

7728596

chore(cli): add static and no-static flags and update createConfig

f1e3302

test(options): update validation test

963244c

alexander-akait reviewed Aug 3, 2020

View reviewed changes

knagaitsev added 4 commits August 3, 2020 14:49

chore(options): fix runnable features ordering for api fallback and f…

dfc8f69

…ix https test

test(options): fix proxy test and normalizeOptions test

da0357f

test(options): disable watching by default for tests and remove conso…

24c82f6

…le log

test(options): add array static publicPath test

769e080

alexander-akait reviewed Aug 4, 2020

View reviewed changes

knagaitsev added 12 commits August 4, 2020 11:26

chore(cli): handle multiple static options differently

b64e513

chore(test): make env variable for chokidar boolean

888b96e

test(cli): make cli test use no static flag

544e0b4

test(stats): disable static for stats test

0191a57

test(options): turn off static by default for tests

51c6b2d

test(options): fix host and port tests

1670984

test(options): only run 1 problematic test

4b65a57

test(options): disable watch mode where possible

0e3e20b

test(options): fix liveReload test

f26069f

chore(server): have only static feature

ce5468b

chore(server): add watch to static feature

efe9bf4

chore(server): add back multiple runnable features for static

b17f327

alexander-akait reviewed Aug 17, 2020

View reviewed changes

knagaitsev changed the title ~~WIP: create static option and remove replaced options~~ BREAKING CHANGES: create static option and remove replaced options Aug 17, 2020

alexander-akait reviewed Aug 17, 2020

View reviewed changes

knagaitsev added 2 commits August 17, 2020 11:18

chore(server): rename static runnable features

20f1fb6

chore(server): rename static feature methods

30b3aca

alexander-akait reviewed Aug 17, 2020

View reviewed changes

alexander-akait approved these changes Aug 17, 2020

View reviewed changes

hiroppy approved these changes Aug 18, 2020

View reviewed changes

hiroppy merged commit c6d2c00 into webpack:v4 Aug 18, 2020

knagaitsev mentioned this pull request Aug 20, 2020

behavior of static and historyApiFallback #2716

Open

ylemkimon mentioned this pull request Sep 23, 2020

fix: contentBase (static) logging #2744

Merged

6 tasks

BREAKING CHANGES: create static option and remove replaced options #2670

BREAKING CHANGES: create static option and remove replaced options #2670

Conversation

knagaitsev commented Jul 4, 2020

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Jul 4, 2020 • edited Loading

Codecov Report

knagaitsev commented Jul 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

alexander-akait Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev commented Aug 3, 2020

alexander-akait commented Aug 3, 2020

alexander-akait Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knagaitsev Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 4, 2020 •

edited

Loading

alexander-akait Jul 6, 2020 •

edited

Loading

alexander-akait Jul 7, 2020 •

edited

Loading

knagaitsev Aug 3, 2020 •

edited

Loading

alexander-akait Aug 4, 2020 •

edited

Loading

alexander-akait Aug 4, 2020 •

edited

Loading

knagaitsev Aug 17, 2020 •

edited

Loading