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

chore(package): update to webpack 2 #1502

Merged
merged 13 commits into from
Apr 2, 2017
Merged

chore(package): update to webpack 2 #1502

merged 13 commits into from
Apr 2, 2017

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Mar 24, 2017

This PR updates webpack to 2 version.

dir_umd_dist: 'dist/umd',
dir_server: 'server',
dir_test: 'test',
Copy link
Member Author

Choose a reason for hiding this comment

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

Cruft.

@@ -35,10 +34,10 @@ const paths = {
base,
src: base.bind(null, config.dir_src),
dist: base.bind(null, config.dir_dist),
test: base.bind(null, config.dir_test),
Copy link
Member Author

Choose a reason for hiding this comment

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

Cruft.

@@ -53,30 +52,27 @@ config = Object.assign({}, config, {
// ----------------------------------
// Compiler Configuration
// ----------------------------------
compiler_devtool: __DEV__ && 'cheap-source-map'
|| __TEST__ && 'cheap-source-map'
Copy link
Member Author

Choose a reason for hiding this comment

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

karma-webpack use inline-source-map

Copy link
Member

Choose a reason for hiding this comment

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

We use karma-webpack-with-fast-source-maps which allows for faster file sourcemaps. Its main trick is that it only re-runs tests that have changed since the last run. With vanilla karma-webpack, if you change one test suite, all 7k+ tests will re-run.

I'm open to changes here but I'd like to address the issue of too many tests taking too long to run while we're at it, or stick with karma-webpack-with-fast-source-maps. Another option I've been considering is using something like the dot reporter to lighten up and speed up test output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted, karma-webpack-with-fast-source-maps works perfectly as I see.

@@ -146,7 +146,7 @@ class ComponentExample extends Component {
getOriginalSourceCode = () => {
const { examplePath } = this.props

if (!this.sourceCode) this.sourceCode = require(`!raw!docs/app/Examples/${examplePath}`)
Copy link
Member Author

Choose a reason for hiding this comment

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

Update loader's usage

@layershifter layershifter force-pushed the chore/webpack branch 2 times, most recently from 3e86db6 to 524cee3 Compare March 24, 2017 13:24
karma.conf.js Outdated
webpack: {
devtool: config.compiler_devtool,
module: Object.assign({}, webpackConfig.module, {
loaders: [
entry: './test/tests.bundle.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

karma-webpack tells we don't need entry, however webpack didn't work without it

package.json Outdated
@@ -108,7 +108,8 @@
"karma-mocha-reporter": "^2.2.1",
"karma-phantomjs-launcher": "^1.0.0",
"karma-phantomjs-shim": "^1.4.0",
"karma-webpack-with-fast-source-maps": "^1.9.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

I've replaced it with karma-webpack and karma-sourcemap-loader because it looks quite outdated.

Copy link
Member

Choose a reason for hiding this comment

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

However, karma-webpack is very slow :( I'll come back to this when I have some more time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

package.json Outdated
"webpack-hot-middleware": "^2.15.0",
"webpack": "^2.3.1",
"webpack-dev-middleware": "^1.10.1",
"webpack-hot-middleware": "^2.17.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated versions

require('imports?document=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports?document=>null!src/lib/isBrowser').default.should.be.false()
require('imports-loader?document=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports-loader?document=>null!src/lib/isBrowser').default.should.be.false()
Copy link
Member Author

Choose a reason for hiding this comment

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

Update loader's usage

)
} else if (!__TEST__) {
webpackConfig.plugins.push(
new webpack.optimize.OccurrenceOrderPlugin(),
new webpack.optimize.DedupePlugin(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Cruft at now

@codecov-io
Copy link

codecov-io commented Mar 24, 2017

Codecov Report

Merging #1502 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1502   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files         141      141           
  Lines        2384     2384           
=======================================
  Hits         2378     2378           
  Misses          6        6

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 751bdde...0995151. Read the comment docs.

config.js Outdated
},
},
__DEV__,
__DEBUG__: !!argv.debug,
Copy link
Member

@levithomason levithomason Mar 25, 2017

Choose a reason for hiding this comment

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

__DEBUG__ is also cruft, note, this build system was copied from one of our production apps and not thoroughly updated, so it has some extra junk that we don't need.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 👍

karma.conf.js Outdated
const formatError = (msg) => {
let haveSeenStack = false
return msg
.split('\n')
Copy link
Member

Choose a reason for hiding this comment

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

This formatter is also borked and I'd like to clean it up with this PR. It used to receive the full stack and filter out blackboxed scripts, however, some time ago it began receiving each line of the stack trace instead. This is why the assertions have had so much extra whitespace in them.

I'll push a fix for this.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out Karma needs a fix first, karma-runner/karma#2627

karma.conf.js Outdated
{
test: /sinon\.js$/,
loader: 'imports?define=>false,require=>false',
loader: 'imports-loader?define=>false,require=>false',
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer required after the sinon 2 update. We just need to rebase this branch.

@@ -157,20 +153,20 @@ webpackConfig.module.loaders = [{
// Local modules can still be enabled (ie for offline development)
// in TEST we need local modules because karma uses a different index.html (no CDNs)
if (__TEST__ || argv.localModules) {
Copy link
Member

Choose a reason for hiding this comment

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

This is more complication that I've never made use of in 2 years (maybe once?). Unless you use it, I say we drop it. It allows for running the project without an internet connection, provided you've already installed node_modules. You'll still get all the network errors due to CDNs failing to load, but, the local node_modules versions will be used instead.

However, this one feature alone requires us to have the style-loader, css-loader, sass-loader, file-loader, and yargs. All these can go away if we drop this feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 👍

@levithomason
Copy link
Member

2.3.2 is out, #1506

@layershifter
Copy link
Member Author

I've pushed a commit:

  • changed webpack version to 2.3.2
  • karma-webpack-with-fast-source-maps returned 😄
  • removed logic with Local Modules

@layershifter
Copy link
Member Author

@levithomason is there something that stops merging of this PR?

@levithomason
Copy link
Member

Testing this now, thanks for the ping.

loader: 'file',
},
]
} else {
Copy link
Member

Choose a reason for hiding this comment

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

We still need the else branch here as it is applicable when not in tests. I'll review it and push an update.

Copy link
Member

Choose a reason for hiding this comment

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

After restoring, the docs gulp task dropped from 1.8m to 24s, woop!

@levithomason
Copy link
Member

I've finished reviewing and updating this PR. I've restored some build performance config we had previously and I also remove some deps that we don't need. Will merge on pass.

@levithomason levithomason merged commit 99fa867 into master Apr 2, 2017
@levithomason levithomason deleted the chore/webpack branch April 2, 2017 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants