-
Notifications
You must be signed in to change notification settings - Fork 54
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
add major features from webpack-dev-server #55
base: master
Are you sure you want to change the base?
Conversation
- compress - proxy - before - after - historyApiFallback (object format) - serveIndex - headers
This looks promising, but I don't have the time to review this completely so I hope somebody else can review. I released the PR as |
options.port = options.port || 10001 | ||
options.headers = options.headers || {} | ||
options.https = options.https || false | ||
options.openPage = options.openPage || '' | ||
options.compress = !!options.compress | ||
options.serveIndex = options.serveIndex || (options.serveIndex === undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new options should be documented in README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, obviously. And some tests added.
"compression": "^1.7.4", | ||
"connect-history-api-fallback": "^1.6.0", | ||
"express": "^4.17.1", | ||
"http-proxy-middleware": "^1.0.0", | ||
"killable": "^1.0.1", | ||
"mime": ">=2.0.3", | ||
"opener": "1" | ||
"opener": "1", | ||
"serve-index": "^1.9.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving all of these to devDependencies and bundling them in the plugin with rollup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok.
} | ||
} | ||
|
||
const runnableFeatures = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of this code is copied from webpack-dev-server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the complex features. Like proxy and feature order.
I don't see how to get feature/bug parity without it.
The license is MIT though, so probably needs a better embedding of the source with the correct attribution.
A happy user of serve({
open: false,
port: 8080,
+ contentBase: __dirname, // I have rollup config in the project root
+ serveIndex: true,
}) If I don't add the
The default |
return next() | ||
} | ||
|
||
serveIndex(item)(req, res, next) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface of serveIndex
is serveIndex(path, options)
. There're some important options there like icons: true
, for which my colleague is mourning now, when migrating from webpack to rollup ;-) How about passing the options for serve-index
down?
This is possible now, using the defaults of serve-index
:
serve({
open: false,
port: 8080,
contentBase: __dirname,
serveIndex: true
})
How about letting us customise the options of serve-index
like this:
serve({
open: false,
port: 8080,
contentBase: __dirname,
serveIndex: { icons: true }
})
It'd mean this code change:
- serveIndex(item)(req, res, next)
+ serveIndex(item, options.serveIndex)(req, res, next)
This feature was included in 2.0.0-beta.0, but it didn't make it to 2.0.0. Are there any plans to include it in an upcoming version? Thank you! |
2.0.2 was released without this feature. Are you going to accept ii? Or do you want to close this PR and wait for a different one? |
The end goal is to make sure it can run
react-scripts
config (almost) unmodified