Extendable base config #884

gauravtiwari · 2017-10-01T10:50:22Z

This PR exposes a base webpack config and a few new API's for updating loaders, plugins and configs so it can be extended from environment instead of separate environment files.

Issues this PR solves:

At the moment if we have to update a config option we need to add that option to all environments instead of just the base environment. This PR adds a base config property on environment itself so we can set common options from environment itself.

environment.config.set('resolve.extensions', ['.foo', '.bar'])
environment.config.set('output.filename', '[name].js')
environment.config.merge({ output: {
    filename: '[name].js'
  }
})

The order of loaders and plugins in webpack config matters and currently we don't have a mechanism to enforce order when adding a loader or plugin. This PR addresses this by providing new API's apart from regular set method.

environment.loaders.prepend('json', jsonLoader)
environment.loaders.insert('json', jsonLoader, { after: 'style'} )

Expose resolve.modules through environment so it can be updated like loaders and plugins.

1. List API

prepend
append
insert

2. Object API

set
get
delete

Examples

// Object API
environment.config.set('output', {
  filename: '[name]-[hash].js'
})

// Using string path notation
environment.config.set('resolve.extensions', ['.foo', '.bar'])
environment.config.set('output.filename', '[name].js')

// Get an object
console.log(environment.config.get('resolve'))

// delete an object
environment.config.delete('output.chunkFilename')

// Loaders list API 
const jsonLoader =  {
  test: /\.json$/,
  exclude: /node_modules/,
  loader: 'json-loader'
}

// (first append and then prepends)
environment.loaders.set('json', jsonLoader)
environment.loaders.prepend('json', jsonLoader)
environment.loaders.insert('json', jsonLoader, { after: 'style'} )

// Updating a loader object
const jsonLoaderConfig = environment.loaders.get('json')
delete jsonLoaderConfig.loader
jsonLoaderConfig.use = ['json-loader']

// Plugins list API - prepend, append, insert
environment.plugins.set('CommonChunkVendor',
  new webpack.optimize.CommonsChunkPlugin({
    name: 'vendor', // Vendor code
    minChunks: (module) => module.context && module.context.indexOf('node_modules') !== -1
  })
)

// This will override previous set
environment.plugins.insert('CommonChunkVendor',
  new webpack.optimize.CommonsChunkPlugin({
    name: 'vendor', // Vendor code
    minChunks: (module) => module.context && module.context.indexOf('node_modules') !== -1
  })
, { before: 'manifest' })

// Insert at position API
const babelLoader = environment.loaders.get('babel')
environment.loaders.insert('coffee', {
  test: /\.coffee(\.erb)?$/,
  use:  babelLoader.use.concat(['coffee-loader'])
})

// Or
environment.loaders.insert('coffee', {
  test: /\.coffee(\.erb)?$/,
  use:  babelLoader.use.concat(['coffee-loader'])
}, { before: 'babel' })


// Resolved modules list API - prepend, append, insert 
environment.resolvedModules.set('vendor', 'vendor')

// Update a plugin
const manifestPlugin = environment.plugins.get('Manifest')
manifestPlugin.opts.writeToFileEmit = false

There are some other chores done here:

like converting all functions to use ES6 syntax
Rename loaders to rules for consistency with webpack convention
Use one syntax for rules i.e. convert loader to use
Expose rules, paths config and merge through environment

Fixes: #850 and #794

Closes: #836

Please review

cc // @molefrog @n0nick @renchap @rossta

javan · 2017-10-01T17:24:27Z

package/asset_host.js

-function removeOuterSlashes(string) {
-  return string.replace(/^\/*/, '').replace(/\/*$/, '')
-}
+const removeOuterSlashes = string =>


Do you prefer this syntax for any particular reason? Personally, I find it much harder to read. And => != ES6 function. Their behavior is slightly different although it doesn't matter in this case.

To me, this is like changing:

class Environment { constructor() { ... } }

to:

const Environment = class { constructor() { ... } }

Nope, just little less curly's {} and parans () for one liner functions 😄

Yeah, for some cases it doesn't do much though.

Happy to change it back if you don't like it 😉 although at some places we have fat arrows so would be ideal to make it consistent in that sense

rossta · 2017-10-01T18:43:39Z

@gauravtiwari With this strategy, how would we modify one of the default plugins? For example, we need to modify the options for the ManifestPlugin, using the workaround cited in #435.

gauravtiwari · 2017-10-01T19:00:42Z

@rossta The plugins are bit complicated since they don't share same API for defining options, which makes merging hard. So at the moment, one has to re-add that plugin with new options and it will override the previous plugin. I am investigating to see if it's possible to add similar API for plugins too.

WoH · 2017-10-06T18:32:38Z

@gauravtiwari maybe store the loaders and plugins as an Array of Objects with a name and a value property. I experimented with something like that here, maybe it is actually a viable approach.

This way, you can add plugins/loaders/other properties via an API, but still have a way to access the predefined rules or plugins webpack does using a name and not relying on webpacker to change the order of the items in the array.

gauravtiwari · 2017-10-10T07:45:21Z

Thanks @WoH I haven't got chance to look at this again but will do at some point this week 👍

gauravtiwari · 2017-10-16T22:33:30Z

Thanks @WoH for pointers. Pushed some updates and updated the description, please take a look 👍

@rossta Plugins can now be added, updated and removed (as loaders and resolvedModules)

const manifestPlugin = environment.plugins.get('Manifest')
manifestPlugin.opts.writeToFileEmit = true

rossta · 2017-10-17T21:34:00Z

@gauravtiwari Thanks for sharing this work. I like that we can configure plugins now. One concern I had with the current Map implementation is that plugin order couldn't be guaranteed and it indeed matters in Webpack; looks like you've addressed the issue with the new ConfigList API.

ytbryan · 2017-10-18T00:30:03Z

Thanks for this @gauravtiwari

Oops sorry. updated the question because I wrote the question wrongly. @gauravtiwari

Which one will take precedence?
environment.loaders.set('x', xloader, 'top')
environment.loaders.set('y', yloader, 0)

gauravtiwari · 2017-10-18T08:15:04Z

@ytbryan The one set later, environment.loaders.set('json', jsonLoader, 1) will be added as second item in the list since you are initialising same loader twice, the first will be simply deleted from the list.

environment.loaders.set('json', jsonLoader, 'top') // now at 0th position in array i.e. head
environment.loaders.set('json', jsonLoader, 1) // now at 1st position in array
// [firstLoader, jsonLoader, ...moreLoaders]

gauravtiwari · 2017-10-18T08:19:42Z

@rossta Yes, now the order can be controlled any way you like, not just inserting at the top or bottom but at any position in the list.

javan · 2017-10-19T12:31:30Z

Reading through your examples, I'm finding the 3rd positioning argument unclear.

// Add json loader at top
environment.loaders.set('json', jsonLoader, 'top')
// Or 3rd in the list
environment.loaders.set('json', jsonLoader, 3)

I'd prefer to see a new set of methods with descriptive names like:

// Insert at the top
environment.loaders.prepend('json', jsonLoader) 

// Insert at the bottom
environment.loaders.append('json', jsonLoader)

// Insert before/after an existing loader
environment.loaders.insert('json', jsonLoader, { before: 'typescript' })
environment.loaders.insert('json', jsonLoader, { after: 'typescript' })

wdyt?

gauravtiwari · 2017-10-19T12:42:48Z

Yeah, I like it and had similar ideas earlier but didn't wanted to introduce a breaking change. Ahh I see, you mean extra set of methods apart from set?

javan · 2017-10-19T12:48:45Z

Yeah, append would be the same as set and rest are new.

gauravtiwari · 2017-11-12T20:29:12Z

@javan Updated the PR and description with new API changes (see examples in PR description)

gauravtiwari · 2017-11-12T20:31:00Z

@ytbryan Could you please give this a try and see if it works as intended?

javan · 2017-11-13T15:50:01Z

package/config_types/config_object.js

+  * @class
+  * @extends { Object }
+*/
+class ConfigObject extends Object {


Extending native Object (and Array above) could have unexpected consequences. I don't have any references to point to, but it's generally not something I do. Do you have any thoughts? Alternatively, these could be bare classes that manage an object internally.

Yeah, I read a bit about it but it seems for our use case it's safe to use since the usage is limited only to webpack config and unlike prototype it doesn't change native objects.

I tried using bare classes with internal object but ran into some gotchas. We basically need native types with some custom methods on it.

Do you think it's bad for our use case?

javan · 2017-11-13T15:57:10Z

It's worth restating what problems / issues these changes resolve in the PR description.

There are a lot of nice API addditions here, but also a lot of changes and increase to the overall footprint. It might be nice to split this into two pull requests: one to resolve the issues, and a second to modernize and refactor (like your package/loaders/* → package/rules/* updates).

gauravtiwari · 2017-11-13T17:16:24Z

Yeah, makes sense. I will do this first and then modernisation later 👍

Remove toWebpackConfig() Expose entries Export all loaders Add webpack merge Revert toWebpackConfig() Add new API's for adding loader and plugins Fix loaders Move base plugins to config

Remove toWebpackConfig() Update API Cleanup More cleanup

Make loaders rules

gauravtiwari · 2017-11-16T19:34:34Z

Going to merge this as is, looks good to me 👍

javan · 2017-11-16T20:15:02Z

package/config_types/config_list.js

+
+  set(key, value) {
+    return this.add({ key, value })
+  }


I think we should deprecate set in favor of append since append / prepend have nice symmetry. For now, let's have set call append instead of the other way around.

Sure, will make a PR

javan · 2017-11-16T20:15:38Z

package/config_types/config_list.js

+  }
+
+  append(key, value) {
+    return this.set(key, value)


This should call add directly instead of set.

gauravtiwari requested a review from javan October 1, 2017 10:50

javan reviewed Oct 1, 2017

View reviewed changes

gauravtiwari mentioned this pull request Oct 11, 2017

Overcomplicated @rails/webpacker package? #895

Closed

gauravtiwari force-pushed the extendable-base-config branch 5 times, most recently from 4091408 to ead5716 Compare October 16, 2017 22:32

gauravtiwari force-pushed the extendable-base-config branch 2 times, most recently from 2283f2d to bbf5abb Compare October 16, 2017 22:42

This was referenced Nov 6, 2017

Release Request in Light of PR #980 #982

Closed

Configure changes to all enviornments #997

Closed

gauravtiwari force-pushed the extendable-base-config branch from bbf5abb to 7408f61 Compare November 8, 2017 07:46

gauravtiwari mentioned this pull request Nov 9, 2017

I can't get resolved_path to work in webpack.yml #984

Closed

gauravtiwari force-pushed the extendable-base-config branch 4 times, most recently from 4ff8e81 to 64e9094 Compare November 11, 2017 21:05

gauravtiwari force-pushed the extendable-base-config branch from 64e9094 to 0b4bbca Compare November 12, 2017 20:13

gauravtiwari force-pushed the extendable-base-config branch 3 times, most recently from d89f6c0 to d73230e Compare November 13, 2017 14:37

javan reviewed Nov 13, 2017

View reviewed changes

gauravtiwari mentioned this pull request Nov 15, 2017

Remove check for webpacker binstubs. (Fixes #988) #1012

Closed

gauravtiwari added 8 commits November 15, 2017 21:42

Change to arrow function

47121fa

Add a base config and extend that in evironments

ba30ef7

Remove toWebpackConfig() Expose entries Export all loaders Add webpack merge Revert toWebpackConfig() Add new API's for adding loader and plugins Fix loaders Move base plugins to config

Use consistent loader configuration

85a6125

Remove toWebpackConfig() Update API Cleanup More cleanup

Rename to rules

9f79827

Make loaders rules

Add API for resolved modules

818672c

Update elm loader

7cfbbe0

Revert to old API and extend native arrays and objects

ebcc01b

Add tests for new config types and utils

145a2d2

gauravtiwari force-pushed the extendable-base-config branch from d73230e to 145a2d2 Compare November 15, 2017 21:42

gauravtiwari added 2 commits November 16, 2017 18:53

Update style loader syntax

59f1865

Set importLoaders

24b1718

gauravtiwari merged commit fb9118e into master Nov 16, 2017

gauravtiwari deleted the extendable-base-config branch November 16, 2017 19:34

gauravtiwari mentioned this pull request Nov 16, 2017

Option to install or export default webpack config from @rails/webpacker? #794

Closed

javan reviewed Nov 16, 2017

View reviewed changes

gauravtiwari mentioned this pull request Nov 16, 2017

adding sourcemaps to css in case of HMR (in dev-mode) #856

Closed

stevenharman mentioned this pull request Nov 18, 2017

Object.entries not available on Node 6 #1021

Closed

Extendable base config #884

Extendable base config #884

Uh oh!

Conversation

gauravtiwari commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues this PR solves:

1. List API

2. Object API

Examples

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rossta commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gauravtiwari commented Oct 1, 2017

Uh oh!

WoH commented Oct 6, 2017

Uh oh!

gauravtiwari commented Oct 10, 2017

Uh oh!

gauravtiwari commented Oct 16, 2017

Uh oh!

rossta commented Oct 17, 2017

Uh oh!

ytbryan commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gauravtiwari commented Oct 18, 2017

Uh oh!

gauravtiwari commented Oct 18, 2017

Uh oh!

javan commented Oct 19, 2017

Uh oh!

gauravtiwari commented Oct 19, 2017

Uh oh!

javan commented Oct 19, 2017

Uh oh!

gauravtiwari commented Nov 12, 2017

Uh oh!

gauravtiwari commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javan commented Nov 13, 2017

Uh oh!

gauravtiwari commented Nov 13, 2017

Uh oh!

gauravtiwari commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gauravtiwari commented Oct 1, 2017 •

edited

Loading

rossta commented Oct 1, 2017 •

edited

Loading

ytbryan commented Oct 18, 2017 •

edited

Loading