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

feat(server): add contentBasePublicPath option #2150

Merged
merged 1 commit into from Dec 5, 2019
Merged

feat(server): add contentBasePublicPath option #2150

merged 1 commit into from Dec 5, 2019

Conversation

iamandrewluca
Copy link
Contributor

@iamandrewluca iamandrewluca commented Jul 24, 2019

Tell the server at what URL to serve devServer.contentBase. If there was a file assets/manifest.json, it would be served at /serve-content-base-at-this-url/manifest.json

webpack.config.js

module.exports = {
  //...
  devServer: {
    contentBase: path.join(__dirname, 'assets'),
    contentBasePublicPath: '/serve-content-base-at-this-url'
  }
};

Motivation / Use-Case

Now webpack-dev-server serves static files (contentBase) from root ignoring publicPath
Using added option we can serve static files from publicPath
This use case is needed in create-react-app, when want to serve all files from publicPath
doesn't matter in development or production

https://github.com/iamandrewluca/plsdelete-wds-use-case
Above repo will be deleted because does not reflect current changes after review.
From start the option was intended to be contentBasePrependPublic: boolean

Breaking Changes

No breaking changes

Related

facebook/create-react-app#7259
facebook/create-react-app#6135
facebook/create-react-app#6280

@jsf-clabot
Copy link

jsf-clabot commented Jul 24, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented Jul 24, 2019

Codecov Report

Merging #2150 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2150      +/-   ##
==========================================
+ Coverage   93.86%   93.88%   +0.01%     
==========================================
  Files          34       34              
  Lines        1288     1291       +3     
  Branches      367      368       +1     
==========================================
+ Hits         1209     1212       +3     
  Misses         78       78              
  Partials        1        1
Impacted Files Coverage Δ
lib/Server.js 97.33% <100%> (+0.01%) ⬆️
lib/utils/normalizeOptions.js 100% <100%> (ø) ⬆️

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 bd6783a...cbb5888. Read the comment docs.

lib/Server.js Outdated Show resolved Hide resolved
lib/Server.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Please provide repo example with use case

@alexander-akait
Copy link
Member

alexander-akait commented Jul 24, 2019

Also you need accept CLA

lib/Server.js Outdated Show resolved Hide resolved
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 24, 2019

I signed CLA!

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 24, 2019

https://github.com/iamandrewluca/plsdelete-wds-use-case

Set contentBasePrependPublic to false and
publicPath is /test/ but manifest.json is served as /manifest.json
Set contentBasePrependPublic to true and manifest.json is served as /test/manifest.json
If we have some public assets, they should be served from publicPath
This allow to make development and production closer to each other as a configuration.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 25, 2019

sounds confused, you want handle static and public directory from one directory, but it is not good idea, why not use publicPath only? Please clarify problem, still can't understand what you want

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 25, 2019

for example in create-react-app, you can set env PUBLIC_PATH that allows you to serve app from desired path at development time, but static resources are still served from root.

In this issue is described the use case with example
facebook/create-react-app#6135
https://github.com/PavelPolyakov/CRA-PUBLIC_URL-demo

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 25, 2019

you want handle static and public directory from one directory

as I know publicPath does not represents a directory, it's a path segmengt at which your app is served. and all compiled in memory resources are served at that path, and take precedence over static resources (contentBase)

@alexander-akait
Copy link
Member

alexander-akait commented Jul 25, 2019

Maybe contentBasePublicPath is better name for this case?

lib/Server.js Outdated Show resolved Hide resolved
@iamandrewluca iamandrewluca changed the title feat(server): add contentBasePrependPublic option feat(server): add contentBasePublicPath option Jul 25, 2019
lib/Server.js Outdated Show resolved Hide resolved
lib/Server.js Show resolved Hide resolved
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 25, 2019

Now I'm strugling with settings contentBasePublicPath a correct value so app.get will handle correctly express.static

@alexander-akait
Copy link
Member

alexander-akait commented Jul 25, 2019

For example:

express.static('public')

handle public directory

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 25, 2019

In my app I set contentBasePublicPath: /^\/test\/.*/,
but my files are not served under test path (ex: /test/manifest.json)
If using app.use instead of app.get it serves correctly

@alexander-akait
Copy link
Member

alexander-akait commented Jul 25, 2019

Use contentBasePublicPath: 'test', it is not route, it is directory, maybe we should get better name here, anyway let's implement logic and then choose better name

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 25, 2019

I think we misunderstood each other.
contentBase is an absolute directory from where to serve
contentBasePublicPath is a route at where to serve

/home/me/project/assets/
  manifest.json
{
  contentBase: '/home/me/project/assets/',
  contentBasePublicPath: '/url-path',
}
localhost:3000/url-path/manifest.json

@alexander-akait
Copy link
Member

alexander-akait commented Jul 25, 2019

oh, i see 👍

@iamandrewluca iamandrewluca changed the title feat(server): add contentBasePublicPath option WIP: feat(server): add contentBasePublicPath option Jul 29, 2019
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jul 29, 2019

@evilebottnawi is there any reason why app.get was used instead of app.use for serving static content?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 29, 2019

Because we should handle only GET

@iamandrewluca iamandrewluca requested a review from knagaitsev as a code owner Sep 16, 2019
@alexander-akait
Copy link
Member

alexander-akait commented Nov 29, 2019

@Flydiverny in my todo, next week will be new release, we need help here #2313 for release

@alexander-akait
Copy link
Member

alexander-akait commented Dec 4, 2019

/cc @hiroppy can you look at this, i want to merge and do new release

lib/utils/normalizeOptions.js Outdated Show resolved Hide resolved
hiroppy
hiroppy previously approved these changes Dec 4, 2019
Copy link
Member

@hiroppy hiroppy left a comment

LGTM with nits

@iamandrewluca iamandrewluca dismissed stale reviews from hiroppy and alexander-akait via cd96602 Dec 4, 2019
Tell the server at what URL to serve `devServer.contentBase`. If there was a file `assets/manifest.json`, it would be served at `/serve-content-base-at-this-url/manifest.json`

__webpack.config.js__

```javascript
module.exports = {
  //...
  devServer: {
    contentBase: path.join(__dirname, 'assets'),
    contentBasePublicPath: '/serve-content-base-at-this-url'
  }
};
```

Now `webpack-dev-server` serves static files (`contentBase`) from root ignoring `publicPath`
Using added option we can serve static files from `publicPath`
This use case is needed in create-react-app, when want to serve all files from `publicPath`
doesn't matter in development or production

facebook/create-react-app#7259
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Dec 4, 2019

@hiroppy it's done!

hiroppy
hiroppy approved these changes Dec 5, 2019
Copy link
Member

@hiroppy hiroppy left a comment

thanks!

@alexander-akait alexander-akait merged commit cee700d into webpack:master Dec 5, 2019
17 checks passed
@alexander-akait
Copy link
Member

alexander-akait commented Dec 5, 2019

Sorry for big delay, we really have very many issues and very few developers

@iamandrewluca iamandrewluca deleted the contentBasePrependPublic branch Dec 5, 2019
@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Dec 5, 2019

No problem, we can still do it! 😄

@raix
Copy link
Contributor

raix commented Dec 5, 2019

Thank you :)

@iamandrewluca
Copy link
Contributor Author

iamandrewluca commented Jan 31, 2020

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.

None yet

8 participants