Add new webpack config #105

jvanbruegge · 2017-04-10T15:17:45Z

This resolves #103

TODO:

nickbalestra

Sweet! I've left some questions, mainly as I don't know much about webpack-blocks

nickbalestra · 2017-04-10T15:25:36Z

packages/cycle-scripts/configs/webpack.config.js

+const BabiliPlugin = require('babili-webpack-plugin');
+const path = require('path');
+
+const babelConfig = {


with babelConfig you mean babel-loader config?

yes, but it will be shared with typescript

what will be shared the babel config or the babel-loader config? as in there there are things that are pure babel-loader for webpack related as cacheDirectory: true. Perhpash we can make this more evident and with better naming of what get shared and what not? Otherwise I think we'll be sharing with ts stuff that he doesn't need/support and I'll rather avoid that

Have removed the loader specific bit, as babel would complain

nickbalestra · 2017-04-10T15:28:14Z

packages/cycle-scripts/configs/webpack.config.js

+const HtmlWebpackPlugin = require('html-webpack-plugin');
+const ProgressBarPlugin = require('progress-bar-webpack-plugin');
+const CopyWebpackPlugin = require('copy-webpack-plugin');
+const BabiliPlugin = require('babili-webpack-plugin');


If we use uglify, do we need babili?

nickbalestra · 2017-04-10T15:28:43Z

packages/cycle-scripts/configs/webpack.config.js

+const autoprefixer = require('autoprefixer');
+const webpack = require('webpack');
+const HtmlWebpackPlugin = require('html-webpack-plugin');
+const ProgressBarPlugin = require('progress-bar-webpack-plugin');


What do we need the progress bar for?

nickbalestra · 2017-04-10T15:30:47Z

packages/cycle-scripts/configs/webpack.config.js

+    entryPoint(path.join(process.cwd(), 'src', 'index' + ending)),
+    setOutput(path.join(process.cwd(), 'build', 'bundle.[hash].js')),
+    babel(),
+    sass(),


Can we leave css related configs to a later PR where we just deal with that ? (saas, autoprefixer,..)

nickbalestra · 2017-04-10T15:33:36Z

packages/cycle-scripts/configs/webpack.config.js

+module.exports = function(language) {
+  const ending = language === 'javascript' ? 'js' : 'ts'
+  const baseConfig = [
+    entryPoint(path.join(process.cwd(), 'src', 'index' + ending)),


do we really need blocks for those? Not sure I see the whole benefits for entryPoint and setOutput

for consitency, I would say yes

nickbalestra · 2017-04-10T15:33:56Z

packages/cycle-scripts/configs/webpack.config.js

+    setOutput(path.join(process.cwd(), 'build', 'bundle.[hash].js')),
+    babel(),
+    sass(),
+    extractText('[name].[contenthash].css', 'text/x-sass'),


What is this for?

extract the css out of the webpack bundle

nickbalestra · 2017-04-10T15:34:56Z

packages/cycle-scripts/configs/webpack.config.js

+    ]),
+    env('development', [
+        devServer({}, require.resolve('react-dev-utils/webpackHotDevClient')),
+        sourceMaps()


what is the sourceMaps default?

cheap-module-source-map
never had issues with them

Cool, yes that's the one I also use them, just wanted to learn about its preset. We should comment those things above them, I think will be super valuable for people that don't know blocks

nickbalestra · 2017-04-10T15:35:45Z

packages/cycle-scripts/configs/webpack.config.js

+    ]),
+    env('production', [
+        addPlugins([
+          new webpack.optimize.UglifyJsPlugin(),


I guess we want Uglify to also work with maps as per our sourcemaps settings perhaps?

Are source maps needed in production? Currently I only added them in the dev environment

nickbalestra · 2017-04-10T15:42:32Z

packages/cycle-scripts/configs/webpack.config.js

+// https://github.com/vuejs/vue-loader/issues/666
+process.noDeprecation = true
+
+const { createConfig, defineConstants, env, entryPoint, setOutput, sourceMaps, addPlugins } = require('@webpack-blocks/webpack2');


Destructuring assignments are not fully supported in node4. I guess we want to keep support to both 4 and 6 for the moment?

nickbalestra · 2017-04-10T15:57:38Z

I think you also want to add the dependency to the cycle-scripts package.json :)

nickbalestra

Nice. I think this part is pretty clean now

nickbalestra

Not sure seing the release speed of webpack-blocks especially in relation to their dependencies that is something we should go with..

nickbalestra · 2017-04-10T16:11:50Z

packages/cycle-scripts/package.json

@@ -30,12 +30,16 @@
    "cycle-scripts": "./index.js"
  },
  "dependencies": {
-    "babel-core": "^6.24.1",


wow webpack-blocks runs on 6.17.0 of babel-core and 6.2.5 of babel-loader. I knew this wasn't a good idea... :(.
Also seems that updating is not something done on a regular base...just 3 release...my bad I didn't check this earlier

This will still install and use the newest version though

You mean if we keep that dependency? Is that something we really wanna do then?
Tbh, now that I see how it works, is not such a big deal achieve the same thing without block, as mainly is env, devserver, and different plugins for typescrpt. I think you can achieve this easily within a plain webpack config object. What make you so vocal about this? I'm probably missing something as I cannot see such an added value, apart of taking ownership of important dependencies like loaders&co away from you...i must be wrong though...

no, because ^6.24.1 will match every minor and patch update. So the dependencies of the blocks are automaticly kept up to date, unless there is a new major version
This is what npm update is for

A change in the package.json is only for indicating that the update is tested.
Maybe one day Greenkeeper will have monorepo support, then it can be also added to the package.json
But it's not really needed

Yes, but if something cause an issue with a specific vers we cannot lock for example babel-core... anyway. Good job. I'll review the rest later

I very much look forward to that day of greenkeeper supporting nested packages :)

nickbalestra · 2017-04-11T09:15:12Z

I see this still in WIP, I've reviewed everything and so far so good to me!
Looking forward to review the upcoming steps

jvanbruegge · 2017-04-11T09:41:15Z

Yes, was busy finishing my talk :). Will work on it later

jvanbruegge · 2017-04-15T09:07:14Z

tested it, everything works now

jvanbruegge · 2017-04-15T09:11:19Z

Should we make the tsconfig part of the template or not?
Currently it is hidden just like the webpack config. Having it exposed would allow the user to set stricter compiler flags.
Basicly we have three options:

Copy it to the appDir
Hiding it, providing weak defaults (for stronger you would have to eject)
Hiding it, providing strong defaults (for weaker, you would have to eject)

Im currently in favor of the third option

I would set the following flags (as default for option 1 and 3)

noFallthroughCasesInSwitch
noImplicitAny
noImplicitReturns
noImplicitThis
alwaysStrict
strictNullChecks

jvanbruegge · 2017-04-15T09:54:29Z

I removed tslint to be included in the linting PR

jvanbruegge · 2017-04-16T12:50:26Z

I would leave the questions to remove webpack-blocks for an extra PR.
I've tested the eject script and it works just fine.
I had to replace the closure to create the WP config with templates, so I can instanciate the template and write it to a file on eject. This way there are no traces of cycle-scripts left

From my side this PR is done, please have another look @nickbalestra
The only thing left to decide is place and strength of the tsconfig

nickbalestra

My general feeling is that at the end to support both TS and JS + streamLibs we need to do quite few compromise and the whole flavor get slightly over complex compared to having two separate flavors, perhaps with some code duplication, but totally independent in how they can evolve and improve over time, what you think?

nickbalestra · 2017-04-16T13:40:15Z

packages/cycle-scripts/configs/webpack.config.js

@@ -1,4 +1,4 @@
-'use strict'
+module.exports = language => `'use strict'


If I get this right, this is not a configuration file for webpack anymore, but a precompiled template that will compile to a webpack configuration factory? If so filename is perhaps misleading, no?

i can rename it to webpack.config.template.js

nickbalestra · 2017-04-16T13:48:34Z

packages/cycle-scripts/scripts/eject.js

-    start: 'node scripts/start.js',
-    test: 'node scripts/test.js',
-    build: 'node scripts/build.js'
+    start: 'NODE_ENV=development webpack-dev-server --config webpack.config.js',


So eject users at the end will get a plain webpack config...? 😛

Yes, no traces of the flavor

No sure we did discussed about this, or I missed this?

not sure, but at least I dont want any external scripts in my git

I don't think that this is the right approach we should use for putting things into PR and deciding what should be a feature and what's not, there are issue to discuss this...

ok, i will add them back in and make a different PR

nickbalestra · 2017-04-16T13:52:57Z

packages/cycle-scripts/scripts/eject.js

-    test: 'node scripts/test.js',
-    build: 'node scripts/build.js'
+    start: 'NODE_ENV=development webpack-dev-server --config webpack.config.js',
+    build: 'NODE_ENV=production webpack --config webpack.config.js'


This will lead to a different experience, no? ie: no more measureFileSizesBeforeBuild

We can look into creating/searching for a webpack plugin for this

jvanbruegge · 2017-04-16T14:41:18Z

what abou the tsconfig?

nickbalestra · 2017-04-16T14:51:17Z

First of all, thank you a lot for the time you putting in there, I truly appreciated!

But I think is important to make few considerations before more work gets done in this directions.

Things should be extensively discussed within issues. PRs scope shouldn't just be extended without prior discussion, as this could easily lead to hard to merge PRs, and therefore waste od work/time by contributors and maintainers, and no ones wants that.
Discussions should be done with the end user in mind, this is not our tool, or something we do for our own personal usage , you can build your own flavor for that and we already have a repo and CCA support this scenario, but here we are thinking about core-flavor. Therefore I suggest we try avoid rationales like "not sure, but at least I don't want any external scripts in my git" they don't provide value in such context.

Because of this I suggest we do the following:

revert all the pr merged into master lately
every diverse work will for the moment purely focus on the deversity release (that is not related to language, but merely to streamLib).
Every attempt to merge languages into core, should be done in a separate experimental branch.

As a first thing in this direction probably CCA could simply prompt user for streamLib only and pass that info to flavors, core and non-core.

jvanbruegge · 2017-04-16T15:59:30Z

I did not think this is an issue, I thought eject means leave completely. Anyway I created #107 for this. I also meant that I (as user) would not want random scripts in my git, not me personally.

I dont think we have to revert anything, we would not win anything. The eject works as expected and has now the cycle scripts were they were before.
To release CCA Diversity, you would just have to merge this PR and maybe #106, everything is ready to go. The build and server run with your custom start and build script. I used template strings for the webpack config for two reasons. First, you will get a normal webpack config that you can also use with the webpack cli. Second, I would have to know which language to use for the closure, but we erase this information on eject. Another options would be to leave the typescript config in there even in the Javascript case, but I think the current solution is better.

The only thing I need to know is if we should provide weak or strong defaults for the typescript config, then this is ready for publish. Currently we have the config hidden with weak defaults. I would make the defaults stronger, as they help to prevent run time issues. The other option would be to just copy it to the app dir and let the user change the defaults

nickbalestra · 2017-04-17T09:59:06Z

We are doing to many things at once.
Feel free to create an experimental branch where we can play with new ideas in this direction, I will close this PR in favor of a leaner, focused work.

For now:

Remove the partial TS support we introduced in master
diversity API ONLY, aka streamLib option (not language) for core flavor.
have core flavor rely on new diversity API.
make sure everything is properly covered by tested.

This will mark the diversity release.

As we saw, having TS and JS together imply making some compromise (from smaller to bigger ones). I think all of those should be properly discussed and evaluated on specific issues, but I think we should first bring cycle-scripts (core flavor) up to date, before deciding to have a language prompt in the core flavor, That was a too early decision without having yet reached diversity and proper DX on the core cycle-scripts.

After diversity release

Focusing on cycle-scripts priorities:

Upgrading cycle-scripts to counter example (I will close Counter example #106 for now as its JS/TS)
Adding cycle-scripts tests
HMR via cycle-restart
CSS, ...

Add new webpack config

93be209

jvanbruegge requested a review from nickbalestra April 10, 2017 15:18

nickbalestra requested changes Apr 10, 2017

View reviewed changes

jvanbruegge added 3 commits April 10, 2017 17:43

Remove unneeded imports

6b18b84

Remove css config

adeb930

Remove ES6 destructuring

9a20d66

jvanbruegge force-pushed the webpack branch from 3e77f3f to 9a20d66 Compare April 10, 2017 15:54

Add comment about default source map type

70d5e11

Add dependencies

714d9dc

nickbalestra approved these changes Apr 10, 2017

View reviewed changes

nickbalestra requested changes Apr 10, 2017

View reviewed changes

nickbalestra approved these changes Apr 10, 2017

View reviewed changes

jvanbruegge added 2 commits April 15, 2017 10:26

Add babel config to typescript

b2b9e2f

Add tsconfig.json

d2b26f0

Remove tslint

b7ed2fd

jvanbruegge changed the title ~~[WIP] Add new webpack config~~ Add new webpack config Apr 16, 2017

nickbalestra reviewed Apr 16, 2017

View reviewed changes

Adjust eject script

39296f0

jvanbruegge force-pushed the webpack branch from c96ec91 to 39296f0 Compare April 16, 2017 15:55

jvanbruegge mentioned this pull request Apr 17, 2017

Typescript & Javascript prompt for cycle-scripts #108

Closed

		@@ -1,4 +1,4 @@
		'use strict'
		module.exports = language => `'use strict'

Add new webpack config #105

Are you sure you want to change the base?

Add new webpack config #105

Conversation

jvanbruegge commented Apr 10, 2017 • edited Loading

nickbalestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickbalestra commented Apr 10, 2017

nickbalestra left a comment

Choose a reason for hiding this comment

nickbalestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanbruegge Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanbruegge Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

jvanbruegge Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

nickbalestra Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickbalestra commented Apr 11, 2017

jvanbruegge commented Apr 11, 2017

jvanbruegge commented Apr 15, 2017

jvanbruegge commented Apr 15, 2017 • edited Loading

jvanbruegge commented Apr 15, 2017

jvanbruegge commented Apr 16, 2017 • edited Loading

nickbalestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanbruegge commented Apr 16, 2017

nickbalestra commented Apr 16, 2017

jvanbruegge commented Apr 16, 2017 • edited Loading

nickbalestra commented Apr 17, 2017

jvanbruegge commented Apr 10, 2017 •

edited

Loading

jvanbruegge Apr 10, 2017 •

edited

Loading

jvanbruegge Apr 10, 2017 •

edited

Loading

jvanbruegge Apr 10, 2017 •

edited

Loading

nickbalestra Apr 10, 2017 •

edited

Loading

jvanbruegge commented Apr 15, 2017 •

edited

Loading

jvanbruegge commented Apr 16, 2017 •

edited

Loading

jvanbruegge commented Apr 16, 2017 •

edited

Loading