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

[WIP] Move webpack config to node package #264

Closed
wants to merge 1 commit into from
Closed

[WIP] Move webpack config to node package #264

wants to merge 1 commit into from

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Apr 17, 2017

This PR moves out webpack config files to it's own package called webpacker-scripts (unfortunately webpacker is taken). The mains reason for this breaking change is - to isolate the code we are going to change always, recently we have received a lot of issues around upgrades.

Most of the setup is same as before, except optional HMR and CSS modules support, which can be toggled as needed. To try this out locally,

gem 'webpacker', github: 'gauravtiwari/webpacker', branch: 'move-to-node-package'

and then run bundle exec rails webpacker:install plus any other integration installers like react, vue or angular if needed. This will setup the app using new webpacker-scripts package.

 "scripts": {
    "postinstall": "npm rebuild node-sass",
    "build": "node_modules/.bin/webpacker build",
    "start": "node_modules/.bin/webpacker start"
  }

screen shot 2017-04-17 at 17 22 12

screen shot 2017-04-17 at 17 22 24

There are two scripts - build and start. Start basically runs webpack-dev-server in development environment and build compiles the js app to an optimised app bundle.

# Procfile
web: bundle exec rails s
webpacker: ./bin/webpack-dev-server

The binstubs are same as before, except the watcher which is deprecated. Also, added a Readme for the webpacker-scripts package, but that's very much in progress.

TODO

  • Update gem readme
  • Pin correct dependencies
  • Nitpicking and Cleanup

@dhh Please review when you get a chance

// cc @ytbryan Feel free to test this out when you have some time. I might have left some bugs 😄

Copy link
Contributor

@nielsslot nielsslot left a comment

Choose a reason for hiding this comment

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

I created a new Rails 5.1 app and tested the installation of Webpacker from this PR. See the inline comments for problems that I encountered.

I like that with this PR the Rails app feels cleaner, the Webpacker specific Webpack configuration is no longer embedded in the Rails app, which should help avoid confusion.

What I miss is a place to put environment independent Webpack configuration. For example we use the ContextReplacementPlugin to only include the locales we care about for moment.js. Another example might be custom alias definitions in the resolve section of webpack.config.js.

With this PR it is possible to merge custom Webpack configuration per environment (development, production and test). Configuration that's independent of environment currently has to be put in each individual environment JS configuration file. Wouldn't it make sense to also support app defined configuration in the shared.js config?

run "./bin/yarn add --dev webpack-dev-server"
Dir.glob(File.join(File.dirname(__FILE__), "../../javascript/webpacker/scripts/*.js")) do |file_path|
name = File.basename(file_path, ".*")
package_json["scripts"].merge!("#{name}" => "node_modules/.bin/webpacker #{name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

A new package.json file won't have a scripts key, so package_json["scripts"] will be nil, which doesn't respond to merge!.

Perhaps add something like package_json["scripts"] ||= {} before the Dir.glob?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks will fix 👍

directory "#{__dir__}/config/webpack", "config/webpack"
directory "#{__dir__}/config/loaders/core", "config/webpack/loaders"
puts "Copying babel and postcss config"
copy_file "#{__dir__}/examples/javascript/.babelrc", ".babelrc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Installing a .babelrc by default seems like a good idea to me, but it does mean that an enthusiastic user that tries to install React through Webpacker afterwards will get a conflict on it. Maybe we can improve the installation of React by not trying to replace the .babelrc but try to add the react preset to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp, that's the plan 👍 I will fix :)

@rafaelfranca
Copy link
Member

Now that the configurations are inside a node package how do I change the configurations if they don't please what I want in my application. Before the path was really clear since all the files were inside the application I could just change the files inside my application. The upgrade path was also clear since upgrading the gem don't necessarily change the behavior of my application.

How are those two concerns addressed now?

@gauravtiwari
Copy link
Member Author

@rafaelfranca Hey, Good point 👍

  1. So, the webpack configuration we ship with webpacker is fairly standard and will work out-of-the-box for many applications, meaning there won't be any changes required. However, for some advanced applications it's possible to update and override webpack config with the new setup -
    Create an environment specific webpack config file, which will merged with node-package config file. We can document the usage for this (already added some in the README).

  2. In terms of upgrade, with ruby gems, most don't require applications to re-run generators. With webpacker a lot of changes that we will introduce involves changes to webpack config files, and therefore it will be required to re-run installers every time the gem is updated to replace old config file with the new one, which could be confusing. We received quite a few issues around this lately. This upgrade path could also cause problems for some who have tweaked the provided webpack config, because the installer will ask them to replace the old one, which they might have to update manually.

With new changes, all these standard config files will be isolated, meaning less confusion around upgrades and no need to re-run installers, but having said that if someone need to add more features to Webpack they can totally do so with their own config files unobtrusively.

@p0wl
Copy link
Contributor

p0wl commented Apr 20, 2017

I'm not sure if this is a good approach. Having seen multiple webpack powered applications grow, the amount of project specific configuration for will grow. I really like the defaults that webpacker provide, but I had to make adjustments on any upgrade (removing erb loader, removing EnvironmentPlugin, configure uglifyjsplugin, ...).

I know that this will still be possible, but as the defaults will be hidden in the node package, it will be much harder to understand what the actual config looks like.

tl;dr: This pr seems to increase the complexity a lot by introducing a default webpack configuration that cannot be changed in the project but must be overwritten.

@gauravtiwari
Copy link
Member Author

@p0wl Yes, absolutely, no two projects are alike. This step is moving towards a direction where we have a set of best practices for webpack isolated in a bundled package and can be maintained in isolation without impacting upgrades or end-user experience. If you have seen next.js or create-react-app they sort of do they same thing for ease and maintainability. Rails is a great example for this - there are some built-in best practices, opinionated defaults and gems that are hidden behind the scene plus power to do advance things where needed.

Your concerns are genuine, but I think with some good documentation this can be easily addressed. Perhaps, we can take out more dependencies into separate installers - like coffee-script, erb etc. so, that it's really bare bones.

@rafaelfranca
Copy link
Member

👍

@justin808
Copy link
Contributor

@powl, check out webpacker_lite. We've stripped webpacker down so it's only the asset helpers. React on Rails will use this stripped-down gem. @p0wl, as you mention, trying to standardize the webpack config is not good for typical production apps.

@gauravtiwari
Copy link
Member Author

gauravtiwari commented Apr 22, 2017

@p0wl Lets discuss the concerns you pointed out so we are more clear and perhaps can fill the gap 😃 Is your main concern is to not have access to the config files? What solution would elevate this problem considering we have a node package that handles webpack config with option to merge additional config? I have also exported out all configs through main module, which could be monkey patched if needed.

import { config, development, production, test } from 'webpacker-scripts'
const merge = require('webpack-merge')
// All path related helpers etc.
console.warn(config)
// All env specific config
console.warn(development)

development.plugins = []
merge(development, {
  plugins: [
    // my own plugins
  ]
})
// config/webpack/development.js
const { development } = require('webpacker-scripts')
const merge = require('webpack-merge')

module.exports = merge(development.entry, {
    reactHot: 'react-hot-loader/patch'
 })

@gauravtiwari gauravtiwari changed the title Move webpack config to node package [WIP] Move webpack config to node package Apr 24, 2017
Cleanup node package and remove copying loaders

Change package name

Rename package

Add webpack dev server

Add webpack-dev-server

Add missing deps

Fix start and build scripts

Cleanup

Bump version

Add utils

Setup gem for dev server in hot mode

Add dev server helper class

Copy scripts to package.json

Fix typo

Remove enabled flag

Bump version

Move dev server option to config and use util helper to add entry points

Bump version

Serve from root

Fix minor typo

Minor update to binstubs

Use block to handle failures

Fix typo

Set default hot server to true

Remove extraneous deps

Minor updates

Order options

Bump version

Fix typo

Fix typo

Fix more typos

Fix asset_host

Don't pass env

USe quotes

Oops add exec

Cleanup and nitpicking

Bump version

Fix minor typo

Bump version

Fix minor typo

Bump version

Exclude javascript package

Remove .js suffix

Fix config file name

Fix bug

Fix bug and bump version

Fix linter

Upgrade packages

Enable toggling for sass and postcss

Add a basic readme

Bump version

Allow env specific override

Bump version

Remove custom config option

Fix typo

Minor typos

Lets use hash in all environments

Use hash

Don't use hash in development

Don't use hash in development

Remove coffee script related deps

Refactor merging app level config and move coffee to it's own installer

Bump version

Add support for react 15.5.0

Upgrade deps

Add table of contents and support stdin

Bump version

Tag console log with webpacker

Bump version

Fix colour

Change color to blue

Minor refactor

Bump version

Fix typo

Start working on readme

Update binstubs

Spawn webpack dev server using after_initialize hook

Update readme

Update readme

Revert railtie change

Merge app config to built-in config

Minor refactor around custom webpack config

Update readme

Bump version

Update syntax

Minor improvements

Update readme

Fix a bug related to relative url

Fix typo
@p0wl
Copy link
Contributor

p0wl commented Apr 26, 2017

My main concern is that by moving the config out of the (user) project, it will be very hard to understand and modify the configuration. create-react-app has the eject mechanism which allows to take over the control of the full configuration.
Based on my experience with webpack larger projects will reach the point where this is necessary, so for me it would be worth to plan for this scenario.
This pull-request seems to aim for the opposite direction: How can we ensure that the webpacker gem can be developed without hazzle for user, which I think is not as much of a problem, once webpacker is considered stable and "ready".

This leads me to another point, there have been multiple issues in which using the gem directly from github has been mentioned (14 so far), which I guess will lead to more problems. Why is is this gem still at version 1.1? Since the release of version 1.1 there have been so much changes and no release.

So my idea would be to start releasing this gem more often and following semantic release and then see if this change (pr) is still needed.

please do not misunderstand, I'm very thankful for all the great stuff that you and the other contributors provide in this project ❤️ , I'm just a bit concerned about the speed that (fundamental) changes to webpacker are introduced without releases, tests and upgrade paths.

@farneman
Copy link

Would it be possible to include a Rake task to generate the necessary files from the current version of the webpacker-scripts package similar to create-react-app's eject command? That way larger projects could break from the provided scripts if they choose to.

@dhh
Copy link
Member

dhh commented Aug 28, 2017

Closing in favor of #706

@dhh dhh closed this Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants