-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Suggested Refinements on the Purpose and Scope of Webpacker #139
Comments
@justin808 this is a great summary and I'd love to help out. The killer part as you mentioned, is that it can be used for legacy Rails projects! I've just got a few questions:
3 & 4) By using the manifest files you shouldn't need to create new helpers. You should be able to use all of the existing ones. |
Good stuff! Some feedback:
We shouldn't need new helpers once we're using a manifest everywhere. Webpacker can patch the built-in Rails helpers to be webpacker-aware. This is what
This shouldn't be needed. Sensible defaults should be set, and they can be overridden in application.rb or one of the environment config files. |
@dleavitt could a Regarding the built-in rails helpers...what if somebody wants to turn off sprockets? Maybe if put the helpers in a separate very lean, webpack 2.2+ focused gem, then Sprockets can refer to that gem to patch the existing helpers? The unusual thing about Webpack integration is that we want to enable a hot reloading server for the assets sometimes, but not always (per ENV value), for development mode. Maybe we can patch the If the options for webpack bundle files should be different, then, IMHO, it would be more confusing to overload the old helper names, and new helper names should be used. I created a label for open issues on React on Rails pertaining to Webpacker and Sprockets. @rupurt has done some work here in issue #696. I recently merged PR 671: Ability to work without Sprockets, which brings up another issue in that there is no |
@justin808 I'm interested to hear more about the use case of completely removing sprockets. We still use sprockets to inline styles for our mailers and then we also use it for the manifest file mapping when we use the helpers. At this point I don't think we should try to remove sprockets but rather leverage what it already gives us. I'd love to know more about how an app works with it removed though i.e. how do you reference assets? Have you had a chance to look at configuring the asset host in development for the dev server? You can guard it with an environment conditional and you don't need to change anything to do with the helpers. https://github.com/rupurt/webpack-sprockets-rails-manifest-plugin#hot-module-replacement |
I don't think folks are talking about removing sprockets, just making sure that webpacker doesn't depend on its presence. |
word, initializer makes more sense for webpack.rb. +1
I think all the functionality you're describing can be added to `compute_asset_path` in a way that's agnostic to the presence of sprockets. Basically if the webpack asset is missing it falls back to whatever the next strategy is (either sprockets, or straight to generating a public path.) This seems like a good idea because you can keep using the same helpers and they mostly just work if you switch from sprockets to webpack. No duplicate code, no duplicate documentation, etc. I'd be curious to get weigh in from someone on the rails core team though.
… On Feb 28, 2017, at 11:54 AM, Justin Gordon ***@***.***> wrote:
@dleavitt <https://github.com/dleavitt> could a /config/webpack.rb by the webpack equivalent of assets.rb? I'd like to see the default configs listed and commented out, so it's clear what can be overridden.
Regarding the built-in rails helpers...what if somebody wants to turn off sprockets? Maybe if put the helpers in a separate very lean, webpack 2.2+ focused gem, then Sprockets can refer to that gem to patch the existing helpers? The unusual thing about Webpack integration is that we want to enable a hot reloading server for the assets sometimes, but not always (per ENV value), for development mode.
Maybe we can patch the javascript_include_tag <https://github.com/rails/rails/blob/be5ddc250874e6c7f4d22e34791f5187448ebb0e/actionview/lib/action_view/helpers/asset_tag_helper.rb#L75> and stylesheet_link_tag <https://github.com/rails/rails/blob/be5ddc250874e6c7f4d22e34791f5187448ebb0e/actionview/lib/action_view/helpers/asset_tag_helper.rb#L111> so that they would first pick a webpack source? Another issue is whether the options that make sense for these helper methods would also apply to webpack files.
If the options for webpack bundle files should be different, then, IMHO, it would be more confusing to overload the old helper names, and new helper names should be used.
I created a label for open issues on React on Rails pertaining to Webpacker and Sprockets <https://github.com/shakacode/react_on_rails/issues?q=is%3Aopen+is%3Aissue+label%3Awebpacker>. @rupurt <https://github.com/rupurt> has done some work here in issue #696 <shakacode/react_on_rails#696>. I recently merged PR 671: Ability to work without Sprockets <shakacode/react_on_rails#671>, which brings up another issue in that there is no assets:precompile if Sprockets is not used. I'm going to update the top description so that it reflects the ability to work without sprockets as a requirement.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEYBvrtPKgDEokcqrLFFXKJyzvRsl8Aks5rhHtkgaJpZM4MOGqZ>.
|
Very cool! It just won't work if you need to be selective about which assets in case you have some old sprockets and some new webpack ones. |
@justin808 I deal with that by using a suffix for webpack generated files i.e. # application.rb
# ...
if ENV["ASSET_HOST"].present?
config.action_controller.asset_host = proc { |source, _request|
if Rails.env.development?
if source =~ /-bundle/
ENV["ASSET_HOST"]
# else
# return nil and use the server host
end
else
ENV["ASSET_HOST"]
end
end
end
# ... |
Just to set realistic expectations here: Rails 5.1 is essentially a wrap. We're not going to add any additional provisions for non-JS webpack helpers in that release. The next stop will be either Rails 5.2 or Rails 6.0. So we'll have a good amount of time (6-9 months, at least) to sort out how it should all feel. I'm not big on overwriting the existing helpers in any case, though. We should provide a smooth transition path from asset pipeline to webpack and allow people to combine them both in a single app. That's the magic behind Webpacker and the javascript pack tag. They can just coexist. So let's continue down along that path. We've discussed the HELLOs in other tickets, btw. For now they stay to provide the bare minimum bootstrapping. Then other gems and efforts can go above and beyond that. |
@dhh good to know. I wrapped up my ideas on the integration into a separate gem e.g. <% # app/views/layouts/application.html.erb %>
<!DOCTYPE html>
<html lang="en">
<head>
<%= stylesheet_link_tag "webpack-application", media: :all %>
</head>
<body>
<%= yield %>
<%= javascript_include_tag "webpack-application" %>
</body>
</html> As I mentioned in the previous comment, the core of this idea can be used to integrate any build system (grunt, rollup, browserify...). The manifest location & disabling asset If we improved sprockets slightly, and reloaded the manifest file when it changes in development, we could also enable asset fingerprints for the development workflow. |
@rupurt I think that's totally fine to keep as a separate project. I don't think Rails out-of-the-box needs to support anything beyond the asset pipeline and webpack, but it's good for us to make it easy to for others to provide integration with other build systems 👍 |
In the early days of React on Rails, we had our generators try to set up CSS modules, linting, hot reloading, etc. This added a TON of extra complexity. There's no reason that the current asset pipeline can't continue to handle non-JS assets as it has always done. That being said, if you turn hot reloading on for JS, the CSS won't hot reload. It might be worth considering if hot reloading (vs. hitting CMD-R) is really worth the maintenance.
💯 agree on keeping the helpers separate! |
@justin808 Should we close this one now? #153 is merged |
Current Description:
I'd suggest the following description:
I had a nice discussion with @gauravtiwari. Based on that discussion, and his example of the asset_pack_tag, I'd like to suggest the following guidelines for this gem:
/app/assets
contains files used by the Asset Pipeline.config/webpack.rb
file./config/webpack.rb
, allow configuration of the controller and view helpers in a convenient manner that does not mandate any particular location of the webpack configuration files nor the subdirectory under the/public
directory for deployment.A gem like the above can be usable for any Rails 4.x and greater project. So the current Rails codebase would love this.
Optional for Rails 5.1
Out of Scope
Documentation
/app/assets/XXXX
as Sprockets supported, and/app/[good-name-for-webpack-stuff]
as for the webpack parts. Some parts of this Rails Guide apply to topics outside of Sprockets, namely Section 4: In Production.Conclusion
A minimal, yet complete, standard mechanism to integrate the webpack JS and CSS output into the Rails views will foster the development of Rails gems to provide integrations of JS frameworks with Rails. An example of such a webpack based framework is React on Rails.
The text was updated successfully, but these errors were encountered: