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

Suggestions on Directory Organization #130

Closed
justin808 opened this issue Feb 27, 2017 · 13 comments
Closed

Suggestions on Directory Organization #130

justin808 opened this issue Feb 27, 2017 · 13 comments

Comments

@justin808
Copy link
Contributor

I'd suggest that webpacker follow the JavaScript, CSS, Images, Fonts organization that we've used for React on Rails. It's really quite simple:

  • All client-related files go under a top level directory called /client

Over the past 1.5+ years, we've found this setup to be extremely popular with our community. This setup has several advantages:

  1. Client-side specialists can use a client-side editor, such as Atom or Webstorm and simply open the /client directory. Under the current structure, one open the "javascript" folder, but then several key files, such as the package.json, node_modules, and webpack configs are not accessible.
  2. Client side linting and testing is more cleanly organized.
  3. Backend specialists can have their editor ignore the /client directory.
  4. The problem of having the node_modules be outside the top level is easily solved by having a minimal package.json like this. The postinstall script will dig into the /client directory and run yarn.
{
  "private": true,
  "scripts": {
    "postinstall": "cd client && yarn",
  }
}

Possibly, the server side could eventually go under /server so the number of top level directories is minimal.

https://github.com/gauravtiwari/webpacker-example-app

2017-02-26_20-54-03

@thewoolleyman
Copy link

+1, it is convenient in many cases to work with all the client code in the same directory tree, not to mention not having to have as many jump-up-and-back-down dot-proliferation in relative path references of webpack and package.json files.

@dleavitt
Copy link
Contributor

dleavitt commented Feb 27, 2017

@justin808 Really dig react-on-rails, you've clearly done a ton of thinking and work on these issues 🙂 . Going to chime in with my two-cents anyway though.

The way webpacker has it now seems about right to me. Railsy but still compatible with node.js/webpack tooling. Some questions about the centralized client directory approach:

  • Having a new top-level directory seems like it should be avoided if at all possible
  • Two package.json files seems kinda weird, right?
  • Where does stuff build to under this setup? public/client?
  • Not sure I understand the problem this solves. Can't webstorm/atom/ide users just open the root directory for the project?

I guess I feel like this is a pretty major departure from the basic approach of the gem so far? Should it maybe be another gem?

@dhh
Copy link
Member

dhh commented Feb 27, 2017 via email

@justin808
Copy link
Contributor Author

justin808 commented Feb 28, 2017

With regards to the current directory structure, the most meaningful change would be to consider changing the name of /app/javascript to something that reflects /app/javascript-and-friends. We actually had exactly this original situation with React on Rails. I started out calling it javascript and some of my engineers suggested the name change to /client. And it's not a huge deal to just "know" that /app/javascript does include non-javascript.

Here are few naming ideas. I'd consider changing the usage of the word "pack" to "bundle" as that's already commonly used in the Webpack space. See #136.

/app/client and /app/client/packs or /app/client/bundles
/app/webpack and /app/webpack/packs or /app/webpack/bundles.

Regardless of changing any default naming, so long as we can configure the locations for:

  • /app/javascripts
  • /node_modules
  • /config/webpack

Then we'll have an easy migration path for Rails 5.0 react_on_rails apps to migrate without huge movements of the files out of the /client directory, which is 100% popular with the existing React on Rails community.

For new React on Rails installs with its generator, I'll do the simplest thing possible, which is to go with whatever is the rails standard. In fact, the yarn and webpacker integration will make the React on Rails install even smoother and easier! I'm excited about that, and I hope to have a PR ready very shortly.

@thewoolleyman
Copy link

+1 to Justin's last comment - these changes seem reasonable, helpful improvements, and to not go against any Rails conventions.

@dhh
Copy link
Member

dhh commented Mar 1, 2017

@justin808 At this time I don't see the default changing to being a component-based pitch, but totally happy to see it be configurable. There are people who like component-based structuring of their serverside Rails code as well, and we should certainly be welcoming of that. So 👍 on making whatever configuration points we need such that you can go with a component-based structure for your plugin and projects.

@justin808
Copy link
Contributor Author

justin808 commented Mar 2, 2017

@dhh Would you be open to having a top level YAML file in a rails project that could allow some of these directory customizations. Something like /rails.yml. I could look into making webpacker customizable using the settings in this file, with the defaults staying as you've defined them.

And then, going forward, we can have this file contain any custom directory mappings and common scripts for tools like Webpacker and React on Rails.

Off the top of my head, the directories and scripts that need the mapping are:

  1. Location of /node_modules
  2. Location of webpack assets, /app/javascripts
  3. Location of webpack configuration files, /config/webpack
  4. A few key scripts for building webpack assets:
    1. Static development build
    2. Static production build
    3. maybe running rails with hot reloading...

Once I get into the modifications, I'll better be able to specify what needs to go into the /rails.yml

Having a YML file will make configuration easier and more obvious than using environment variables for things that are project specific.

@dhh
Copy link
Member

dhh commented Mar 2, 2017

I don't think we need a new configuration file to accomplish this. We already have a good configuration system that has worked well enough for configuring the asset pipeline as well. So let's just extend that.

What are the alternative placements that you have in mind for node_modules or config/webpack?

@justin808
Copy link
Contributor Author

justin808 commented Mar 3, 2017

I don't think we need a new configuration file to accomplish this.

I think it would be nicer if scripts (like the webpack configs) do not have to parse Ruby code. A more generic configuration readable by both the webpack and ruby config files is either JSON or YAML.

How about we allow some optional overrides from the defaults in /package.json. Extra keys are allowed per the guidelines specified here.

What are the alternative placements that you have in mind for node_modules or config/webpack?

We have a large base of production apps that are on the standard of putting all webpack related items to be quarantined in /client. This includes the node modules, the webpack configs, the client side JS, CSS, Images, Fonts, etc., and the "real" package.json used. The top level package.json defers to this directory. The "nice" thing about this organization is that you can use JS editors within the /client directory.


How about if I made a PR where the directory names have the current defaults but can be overridden in /package.json, and the webpacker scripts will check on that?

{
   "rails": {
     "clientFiles":  "./app/javascripts", 
     “deploymentDirectory”:  “./assets/webpack”,
     "nodeModules":  "./node_modules"
  }
  dependencies: {
  }
  // etc.
}

I'm not sure if the location of the /config/webpack directory for the webpack files matters so much as typically scripts in package.json will invoke the webpack executable with the location of the config file like this:

"scripts": {
    "build:client": "webpack --config webpack.client.rails.build.config.js",
}

@dhh
Copy link
Member

dhh commented Mar 3, 2017

I see. I'm not sure this adds that much over just changing the webpacker binstubs, for example. The code is generated, not hidden inside of Rails, so it's all there for the changing. The same goes for the webpack configs themselves.

I was more thinking if there are any configs within Rails, like for the view helpers, that need specialization, we can provide options for that. But otherwise I think it's fine for someone to tailor all the generated files.

@justin808
Copy link
Contributor Author

justin808 commented Mar 4, 2017

Hi @dhh! I'd like to see any yaml or json standard place for a configuration that is not environment specific. The reason is that with JavaScript files, it's quite inconvenient to have to write a ruby binstub script to read a ruby configuration file and to then put whatever value into some environment values.

Current Ruby Way

ruby-config ==> parsing or loading ==> set env value before calling JS file

Proposed way

JS file (and Ruby files) read a standard JSON OR YAML configuration

Details

We could have a simple configuration file (or use the package.json and a rails section), from which we can read values into from our webpack configs and other JavasScript files. While ENV values can work, it's not easy for the simple task of git clone and then running an app.

Plus, I think there's an expectation that env specific files like development.rb will override what's in application.rb. This sort of default and override is commonly done in the database.yml file. The below code in the binstub assumed the value would only be in development.rb

While any gem can create their own standard configuration (as we did by putting EVERYTHING JavasScript under /client) or mechanisms for configuration, I'd like to see something that's more easily readable by JavaScript. Yaml parsing can be done by js-yaml which had almost 14 million downloads last month. So YAML parsing is popular.

For example, consider this:
https://github.com/rails/webpacker/blob/master/lib/install/bin/webpack-dev-server.tt#L20

# Look into the environment file for a non-commented variable declaration
unless File.foreach(File.join(APP_PATH, RAILS_ENV_CONFIG)).detect { |line| line.match(/^\s*[^#]*config\.x\.webpacker\[\:dev_server_host\].*=/) }
  puts "Warning: if you want to use webpack-dev-server, you need to tell Webpacker to serve asset packs from it. Please set config.x.webpacker[:dev_server_host] in #{RAILS_ENV_CONFIG}.\n\n"
end

This could be checking if a value is configured in the YAML or JSON file.

@justin808
Copy link
Contributor Author

justin808 commented Mar 5, 2017

An example of the json suggestion is being implemented by @gauravtiwari in #153. It looks like a great direction based on the code. For example, to read a setting:

const { webpacker } = require('../../package.json').config
const distDir = webpacker.distDir

From https://github.com/rails/webpacker/pull/153/files#diff-67f77b829953e34e910db8d7c0c285c3R9

@gauravtiwari
Copy link
Member

@justin808 We can close this one. #153 is now merged that addresses this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants