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

BREAKING CHANGES: handle publicPath correctly #2671

Merged
merged 10 commits into from
Jul 26, 2020

Conversation

knagaitsev
Copy link
Collaborator

@knagaitsev knagaitsev commented Jul 6, 2020

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

Not yet, should add tests for:

Routes: dev option setting publicPath and index. Setting publicPath in compiler options without setting it for dev middleware. Using single or multi compiler

Motivation / Use-Case

Fixing how publicPath is handled

Requires: webpack/webpack-dev-middleware#674 to work

Breaking Changes

publicPath is no longer set at all by the dev server, the option is retrieved from webpack-dev-middleware, which falls back to webpack output publicPath if there is no dev middleware publicPath

Additional Info

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #2671 into v4 will increase coverage by 0.18%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v4    #2671      +/-   ##
==========================================
+ Coverage   92.67%   92.85%   +0.18%     
==========================================
  Files          37       37              
  Lines        1310     1316       +6     
  Branches      357      354       -3     
==========================================
+ Hits         1214     1222       +8     
+ Misses         91       89       -2     
  Partials        5        5              
Impacted Files Coverage Δ
lib/utils/createConfig.js 94.59% <ø> (-0.47%) ⬇️
lib/utils/routes.js 98.14% <95.45%> (+5.46%) ⬆️

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 861c294...f4bba22. 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.

Ping when it will be ready to review, thanks for help

@knagaitsev knagaitsev changed the title WIP: handle publicPath correctly BREAKING CHANGE: handle publicPath correctly Jul 20, 2020
@knagaitsev
Copy link
Collaborator Author

@evilebottnawi it's ready, the tests here that are failing are from instability fixed in #2680

@knagaitsev knagaitsev changed the title BREAKING CHANGE: handle publicPath correctly BREAKING CHANGES: handle publicPath correctly Jul 20, 2020
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.

Good job

/cc @hiroppy

@hiroppy
Copy link
Member

hiroppy commented Jul 21, 2020

@Loonride please check this error of windows/node-10-canary.

FAIL test/server/utils/routes.test.js (362.772 s)
  ● routes util › multi config › should handle GET request to directory index and list all middleware directories

    Timeout - Async callback was not invoked within the 120000 ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 120000 ms timeout specified by jest.setTimeout.

      at mapper (node_modules/jest-jasmine2/build/queueRunner.js:29:45)

  ● routes util › multi config › should handle GET request to directory index and list all middleware directories

    : Timeout - Async callback was not invoked within the 120000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 120000 ms timeout specified by jest.setTimeout.Error:

      129 |     afterAll(testServer.close);
      130 | 
    > 131 |     it('should handle GET request to directory index and list all middleware directories', (done) => {
          |     ^
      132 |       req.get('/webpack-dev-server').then(({ res }) => {
      133 |         expect(res.headers['content-type']).toEqual('text/html');
      134 |         expect(res.statusCode).toEqual(200);

      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
      at Suite.<anonymous> (test/server/utils/routes.test.js:131:5)
      at Suite.<anonymous> (test/server/utils/routes.test.js:123:3)
      at Object.<anonymous> (test/server/utils/routes.test.js:9:1)

@knagaitsev knagaitsev force-pushed the fix/public-path-handling branch from d779c90 to c479bb1 Compare July 21, 2020 21:25
@alexander-akait
Copy link
Member

@Loonride Need rebase

@knagaitsev knagaitsev force-pushed the fix/public-path-handling branch from c479bb1 to 1e99668 Compare July 22, 2020 17:46
@knagaitsev
Copy link
Collaborator Author

It seems that webpack@5 on windows had an issue with webpack options output.path: '/'. This should be valid behavior, since / is not a valid path on windows, correct?

@knagaitsev
Copy link
Collaborator Author

/cc @evilebottnawi @hiroppy

@alexander-akait
Copy link
Member

/cc @hiroppy

@alexander-akait
Copy link
Member

/cc @hiroppy friendly ping again 😄

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.

4 participants