-
Notifications
You must be signed in to change notification settings - Fork 40
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
refactor: Consolidate disparate Rollup configs into a single Rollup config. #155
Conversation
bdbf317
to
3a13d1d
Compare
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.
Code LGTM, I am just wondering if we want to pull in a few more things from the mpd-parser pr, I guess we could do them separately though.
- Move the banner insertion and uglification into the rollup config, and get rid of the scripts
- Get rid of the test bundle and have karma rollup for us
- Get rid of
test/index.html
and link to the karma debug page
Yeah, suppose we could do that. Sounds good. |
31be781
to
dda56db
Compare
|
@@ -0,0 +1,3 @@ | |||
const pkg = require('../package.json'); | |||
|
|||
module.exports = `/*! @name ${pkg.name} @version ${pkg.version} @copyright ${pkg.author} @license ${pkg.license} */` |
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.
Do we want this on multiple lines like before? seems like a waste of space to me, but I don't feel strongly that it has to be done on the same line.
|
||
const files = new nodeStatic.Server(process.cwd(), {cache: false}); | ||
|
||
const server = http.createServer((request, response) => { | ||
response.setHeader('Cache-Control', 'no-cache,must-revalidate'); | ||
const requestUrl = url.parse('http://' + request.headers.host + request.url); |
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.
Auto redirect /test
to karma server
@@ -11,7 +19,7 @@ module.exports = function(config) { | |||
|
|||
// If no browsers are specified, we enable `karma-detect-browsers` | |||
// this will detect all browsers that are available for testing | |||
if (!config.browsers.length) { | |||
if (config.browsers !== false && !config.browsers.length) { |
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.
passing --no-browsers
causes this to be false rather than an empty array, and we want to respect --no-browsers
for the test:server
aea3664
to
d83ab10
Compare
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.
LGTM
66e4e29
to
03b7c8a
Compare
03b7c8a
to
fb5ad5a
Compare
fb5ad5a
to
c745536
Compare
No description provided.