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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide repo example with use case

@alexander-akait
Copy link
Member

Also you need accept CLA

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

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

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

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
@iamandrewluca
Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

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

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

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

@alexander-akait
Copy link
Member

Because we should handle only GET

@alexander-akait
Copy link
Member

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

@alexander-akait
Copy link
Member

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

hiroppy
hiroppy previously approved these changes Dec 4, 2019
Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits

@iamandrewluca iamandrewluca dismissed stale reviews from hiroppy and alexander-akait via cd96602 December 4, 2019 16:01
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

@hiroppy it's done!

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

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

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

@iamandrewluca iamandrewluca deleted the contentBasePrependPublic branch December 5, 2019 08:39
@iamandrewluca
Copy link
Contributor Author

No problem, we can still do it! 😄

@raix
Copy link
Contributor

raix commented Dec 5, 2019

Thank you :)

@iamandrewluca
Copy link
Contributor Author

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.

8 participants