☮️ Config.merge compatibility #19

aretecode · 2017-03-19T09:44:35Z

▶️ ◀️ mergeable:

"📌" string entry point
[🔌] plugin array

eliperelman · 2017-03-20T14:43:00Z

src/Config.js

@@ -89,6 +90,11 @@ module.exports = class extends ChainedMap {
          }

          case 'entry': {
+            if (typeof value === 'string') {


Can you provide an example of what the intent is with this? Given a string value of entry, what entries does it generate in the config?

eliperelman · 2017-03-20T14:43:14Z

src/Config.js

@@ -89,6 +90,11 @@ module.exports = class extends ChainedMap {
          }

          case 'entry': {
+            if (typeof value === 'string') {
+              const path = require('path');


Move require to the top of the file.

eliperelman · 2017-03-20T14:43:32Z

src/Config.js

+            if (typeof value === 'string') {
+              const path = require('path');
+              const name = path.basename(value);
+              return this.entry(name).merge([value]);


Insert line between declaration and return.

eliperelman · 2017-03-20T14:43:50Z

src/Config.js

+              const path = require('path');
+              const name = path.basename(value);
+              return this.entry(name).merge([value]);
+            }
            return Object


Insert line between conditional and return.

eliperelman · 2017-03-20T14:44:31Z

src/Config.js

@@ -100,6 +106,17 @@ module.exports = class extends ChainedMap {
              .forEach(name => this.plugin(name).merge(value[name]));
          }

+          case 'plugins': {
+            if (Array.isArray(value)) {


This seems to only handle the case when plugins is an array. Is that intended? Is this supposed to fall through to the next case if it's not an array?

yes it is intended since plugins can only be an array https://webpack.js.org/configuration/plugins/#plugins

though we could add a warning if it is not an array?

eliperelman · 2017-03-20T14:45:12Z

src/Config.js

+            if (Array.isArray(value)) {
+              return value
+                .forEach((plugin, index) => {
+                  if (plugin.name) index = plugin.name;


Use multiline conditionals:

if (plugin.name) { // ... }

eliperelman · 2017-03-20T14:45:46Z

src/Config.js

+            if (Array.isArray(value)) {
+              return value
+                .forEach((plugin, index) => {
+                  if (plugin.name) index = plugin.name;


So if there is no name, the index becomes the name, i.e. a number?

eliperelman · 2017-03-20T14:51:25Z

src/Config.js

+              return value
+                .forEach((plugin, index) => {
+                  if (plugin.name) index = plugin.name;
+                  if (isClass(plugin)) this.plugin(index).plugin(plugin);


Instead of pulling in another dependency, couldn't this just check if the plugin is a function?

if (typeof plugin === 'function') { this.plugin(index).plugin(plugin); }

eliperelman · 2017-03-20T14:52:16Z

src/Config.js

+            if (Array.isArray(value)) {
+              return value
+                .forEach((plugin, index) => {
+                  if (plugin.name) index = plugin.name;


To save from overwriting the index variable, use a variable, probably name:

const name = plugin.name || index;

aretecode · 2017-03-28T06:29:38Z

src/Config.js

+                .forEach((plugin, index) => {
+                  if (plugin.name) index = plugin.name;
+                  if (isClass(plugin)) this.plugin(index).plugin(plugin);
+                  else this.plugin(index).init((args) => plugin);


it would be nice to decide on a function here to tap args again if the plugin provides some sort of standard interface for it.

📦⬇ remove is-class dep

👕⚒ fixed linting according to review 🎁 added string merging for output

📝 added todo for using latest webpack as it requires absolute paths

aretecode · 2017-03-28T07:06:10Z

@eliperelman updated

eliperelman · 2017-03-28T14:15:30Z

package.json

@@ -23,6 +23,6 @@
  },
  "devDependencies": {
    "ava": "^0.18.2",
-    "webpack": "^2.2.1"
+    "webpack": "2.2.1"


We can leave this change out.

if it isn't changed, it breaks unless you install with yarn unless the absolute paths are fixed

eliperelman · 2017-03-28T14:16:27Z

src/Config.js

@@ -1,3 +1,4 @@
+const path = require('path');


Destructure this into:

const { basename, dirname } = require('path');

eliperelman · 2017-03-28T14:20:52Z

src/Config.js

+           *
+           */
+          case 'output': {
+            if (typeof value === 'string') {


return this.output.merge(typeof value !== 'string' ? value : { path: dirname(value), filename: basename(value) });

eliperelman · 2017-03-28T14:28:15Z

src/Config.js

          case 'entry': {
+            if (typeof value === 'string') {


I can't understand the rationale for this one. Why should './src/front/index.js' map to an entry point named index? Many directories could contain a file named index.js, without the justification that they should be bundled together. It seems the using the parent directory name would be a better fit, e.g. front entry. Making this conventional I think will have unintended consequences.

Just need to understand why we should go this route as opposed to using a different name (or whether accepting a string at all here is a good idea).

eliperelman · 2017-03-28T14:33:51Z

src/Config.js

+           * merge an array of plugins
+           */
+          case 'plugins': {
+            if (Array.isArray(value)) {


I'm still having trouble seeing why we need this to be an array, when we already have an dictionary collection. It's essentially this:

plugin: { name: { plugin: Plugin, args: [args] } }

versus something like this I'm assuming:

plugins: [ { name, plugin: Plugin, args: [args] } ]

How is the current plugin interface lacking?

#21 (comment)
if I merge in a config of existing plugins, it does not merge properly

by plugin interface I mean if there is a plugin that results from a .toConfig, or, from merging in an existing webpack config

eliperelman · 2017-03-28T14:35:10Z

test/Config.js

@@ -150,6 +216,7 @@ test('validate with entry', t => {
  t.is(errors.length, 0);
 });

+test.todo('resolve entry & output with latest webpack');


There should be a space in-between tests.

eliperelman · 2017-03-28T14:36:47Z

test/Config.js

@@ -54,6 +54,72 @@ test('entry', t => {
  t.deepEqual(config.entryPoints.get('index').values(), ['babel-polyfill', 'src/index.js']);
 });

+test('merge entry as string with existing entries, with the same prop', t => {
+  const configObj = {


This variable doesn't appear to be used.

eliperelman · 2017-03-28T14:38:26Z

test/Config.js

+
+  config.merge(configObj);
+
+  const backToObj = config.toConfig();


This doesn't appear to be used.

eliperelman · 2017-03-28T14:39:30Z

test/Config.js

+    entry: 'src/index.js'
+  };
+  const config = new Config();
+  config.merge(configObj);


There should be a blank line between variables and other statements in a function.

eliperelman · 2017-03-28T14:40:17Z

src/Module.js

@@ -28,6 +28,7 @@ module.exports = class extends ChainedMap {
        const value = obj[key];

        switch (key) {
+          case 'rules':


Rules doesn't have a high-level API, so I'm not sure why this is needed. All other high-level API interfaces are singular.

#21 (comment)

neutrinojs/neutrino#151 - painfully debug typo in webpack-chain neutrinojs/webpack-chain#21 - update webpack-chain pr neutrinojs/webpack-chain#19 - updating foldername to fliphub from fliphub2 - example/temps working - set rollupdefaults to preset defaults - handle replacing things in rollup that are webpack and fusebox only with entry points - adding resolveall to rollup - update webpack-chain pr again - update presets to use fuse-box instead of fsbx - better resolving with prettier relative paths - splitting and updating node example - splitting and updating canadas example - add .name to chained by default - add outputToString method on bundler config for support from webpack to fusebox and rollup - fix double slash in alias to fusebox - add PresetWeb to move react/inferno/web out of PresetBabel - change .resolve.alias to .alias for fusebox and rollup - add rollup-plugin-alias - updating empty example - update rollup defaults to resolve output paths from webpack... - html preset part 3 - pr updating fuse-box spinner fuse-box/fuse-box#442 - decorating fliplog with .color(text) shorthands - making fliplog implement PSR-3 - making fliplog callable as a fn - make extend-hidden object - fix extra : used in fliplog if no text or title - make does-include module with {any, all} - update fliphub-core pkg json - update flipfile pkgjson - cleaning some folders like yuge into playground - cleaning old flipchain from modules/next - adding missing pkjsons for lerna - pkgjson for emoji-by-name - updating fosho to use extend-hidden - rename extend-hidden to expose-hidden - update inspector-gadget docs - publish packages - make repo for fliplog - update readme badge url for codestyle - update gitter room - update gitter badge - fix some screenshots - publish the modules - push to github # part 55 - mar 27 17 - move .each and .all to izz from fosho - testing other bundlers to ensure output for all :-) - make fusebox default set flipto - make bundlerflipper re-init and re-merge if to is changed - add del to flipfile - add log to fliphub exports - clean neutrino-preset - update neutrino preset toWebpack fn as workflow changes where the config and bundler api are - continue on examples - fix toConfig for contexts -> have them use bundler.api.config if available - neutrino with newest version breaking with immutables, revert some changes, make issue # part 54 - mar 26 17 - going to add tests for bundling, make sure it all works, then good to go :-) - remove generated mocks woops - update todos, remove dist - add isRizzle, add power assert, convert all fuse tests to 10x smaller scoped - test preset inheritence - test multiple apps - test default to webpack - test flipto rollup - verbose reporting for fosho logging diffs and failures - build tests - fix calling .build on context on buildSync op - adding buildSync test - adding buildFast test - adding build output exists test - adding auto calling setup in ops if it was not explicitly called - add safety to stop the spinner in fliplog - starting defaults - adding defaults prop to configs - adding to configDefaulter - adding monorepo mode for resolving easier - fixing alias resolve - fixing preset merging when args exist and they are undefined which triggered falsy condition and did not set them over - extending fusebox default preset - fix regression in merging configs in bundler flipper

eliperelman · 2017-09-13T15:31:15Z

Closing as stale, feel free to open a new PR, maybe where we can have an import method to transform a webpack config before merging.

aretecode added 4 commits March 19, 2017 02:39

mergeable string entry point + plugin array

1272941

📦⬆ added izz-class dep to keep is simple

728afbd

👕 indentation

962a21b

use izz-class > izz

07f695a

eliperelman suggested changes Mar 20, 2017

View reviewed changes

fix rules typo on merge for module

38f8d78

aretecode commented Mar 28, 2017

View reviewed changes

aretecode added 4 commits March 28, 2017 00:03

📦 fixed webpack version from ^

9cf2de3

📦⬇ remove is-class dep

ℹ️️ add example comments for merging in Config

6c77377

👕⚒ fixed linting according to review 🎁 added string merging for output

🔬 add tests for expected results of merging

2844577

📝 added todo for using latest webpack as it requires absolute paths

📝 added todo for using latest webpack as it requires absolute paths

e025040

eliperelman suggested changes Mar 28, 2017

View reviewed changes

aretecode added 2 commits March 28, 2017 13:41

update according to review

e89bc71

fallback to merge if not a string for output

c381b88

eliperelman closed this Sep 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

☮️ Config.merge compatibility #19

☮️ Config.merge compatibility #19

aretecode commented Mar 19, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

aretecode Mar 28, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

eliperelman Mar 20, 2017

aretecode Mar 28, 2017

aretecode commented Mar 28, 2017

eliperelman Mar 28, 2017

aretecode Mar 28, 2017

eliperelman Mar 28, 2017

eliperelman Mar 28, 2017

eliperelman Mar 28, 2017

eliperelman Mar 28, 2017

aretecode Mar 28, 2017

eliperelman Mar 28, 2017

eliperelman Mar 28, 2017

aretecode Mar 28, 2017

eliperelman Mar 28, 2017

aretecode Mar 28, 2017

eliperelman Mar 28, 2017

eliperelman Mar 28, 2017

aretecode Mar 28, 2017

eliperelman commented Sep 13, 2017


		config.merge(configObj);

		const backToObj = config.toConfig();

☮️ Config.merge compatibility #19

☮️ Config.merge compatibility #19

Conversation

aretecode commented Mar 19, 2017

▶️ ◀️ mergeable:

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

aretecode commented Mar 28, 2017

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

eliperelman commented Sep 13, 2017