-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Asset Management #2311
Comments
I'm taking a bit of a different approach to this problem, as it's been quite long-running, and had a few mis-starts and I think it really needs some dedicated time to dig into it and find the right solution. I'm going to be taking a week to work on this in Q1 2023, whilst I can't guarantee we'll solve it all completely, I suspect this will be enough to unblock us. My plan is to use this issue as a bit of a development diary, so others can follow along. Some opening questions as I make sure I understand the full scope: |
You can use dart-sassc to fix the sassc dependency |
Rails 7 start using importmap and drop webpack by default. we might also need think about this |
|
@nickcharlton saw this pop up not too long ago and wondered if the strategy here is something that could be used w/Administrate: https://dev.to/adrianthedev/how-to-bundle-assets-in-a-rails-engine-ilp |
I agree with @jayroh, compile at committing/publishing time and let the user import the compiled assets with the method they want. |
Coming here from #2311, is there a way to move the build into the build phase at this time, or will it require some work on the gem and a new version published with baked assets? |
This PR safe me to left sassc. i don't really know assert pipelines. but it works for me on rails 7 |
Ah I meant to leave a comment in this issue when I made my PR, but I couldn’t find it anymore. Thank you @jubilee2 I have my fork running in production now and it works nicely, so it feels to me like, for now, sassc is solved, but maybe I missed something. Could someone do a review maybe? |
Instead of putting the javascript at the end of the body, I suggest we move it to the head and change the way we attach event handlers from using the jquery function to using the on function with multiple events. This way, we can make sure our code runs whenever the page is loaded or updated by turbolinks or turbo. For example, instead of writing what I think can move to head
for following js $(function() {
.....
}); we can write $(document).on('ready page:load turbolinks:load turbo:load', function() {
....
}); I know this might cause some issues with some plugins, but I think it’s worth it |
@nickcharlton, I agree with @jayroh above (#2311 (comment)). Asset management in Rails is a moving target. We might be able to adapt to potential host apps now, but then we are almost guaranteed to have to adapt again in the future (which surprises will Rails 8 bring?). Therefore, moving the effort to the developer's side, baking the assets fully compiled into the gem, and publishing them as a single file of vanilla CSS and a single file of vanilla JS, seems to me like the most sustainable approach. |
Actually, looks like there's a PR open proposing this approach: #2397. I'm going to spend some time supporting it. |
Yeah, agreed. I think shipping with compiled assets has a lot of merit and is the best way to approach this, both for now and in the future. I think we just need to figure out what the upgrade path looks like so that we can document that when we get to the point of making a release. For those following this, if you could test out #2397 on your applications (and comment there) that'd be wonderful. |
Taking inspiration from how Avo handles their gem assets, this bundles a compiled version of our assets inside the gem. We're using `jsbundling-rails` with `esbuild`, along with `cssbundling-rails` with `saas` to build Administrate's asset files, which is the closest of the newer approaches for handling assets in Rails as to what we'd been doing previously. We then remove `jquery-rails`, `sassc-rails` and `selectize-rails` gems which are replaced with the equivalents from `npm`. When releasing, we'll need to run `yarn run build` and `yarn run build:css` to compile the assets before building the gem which will now ship the contents of `app/assets/builds` with it. For someone using Administrate, they'll no longer need to compile the assets shipped by it as these will now be ready-to-use `.js` and `.css` files and any asset bundling approaches we choose (or need to, because they've changed upstream in Rails) should no longer have an impact on users of the gem. This is part of: #2311 https://avohq.io/blog/how-to-bundle-assets-in-a-rails-engine Co-authored-by: Pablo Brasero <pablo@pablobm.com>
#2397 has now been merged in. I'm going to close this now, as we've got a solution. I'm going to start releasing some pre-release versions of the gem in the coming days, as I'm sure I'm about to break stuff for people, and so we can get feedback and make sure there's nice migration help sorted out. But those will be best tracked in a new issue. |
I'm working on fork on
but problem is that, with this way the gem are not able to install with is there any work around to this solution? Edit: It was different issue I can able to install the adminstrate engine through github. :p |
One of the biggest problems we have is handling assets. We've got a few CSS and JS requirements and as it stands, we assume you're using Sprockets. This isn't really the case any more, with lots of folks using Tailwind, Webpacker and new bundling approaches in their projects. All of these are reasonable and using Administrate shouldn't be as annoying or difficult as it currently might be.
There's been a lot of issues opened, but also not much traction in coming up with a solution that can work for all. This issue will pull together all existing issues, describe the current state of things and outline a plan to make everything that should work, work.
Existing issues:
Proposed solutions:
importmap-rails
#2446Current plan:
The text was updated successfully, but these errors were encountered: