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

Attempt at publishing the gem with compiled assets #2397

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Attempt at publishing the gem with compiled assets #2397

merged 1 commit into from
Feb 2, 2024

Conversation

elias19r
Copy link
Contributor

@elias19r elias19r commented Jul 7, 2023

Issue #2311

I've been looking at how Avo handles the gem's assets and decided to
try a similar solution for Administrate's issue with asset management.

In this PR, I'm using jsbundling-rails with esbuild and
cssbundling-rails with saas to build Administrate's asset files.
For that, I've removed the jquery-rails, saasc-rails, and
selectize-rails gems and replaced them with NPM packages, updating
imports and moving files as needed.

The goal is to be able to run yarn run build and yarn run build:css
to build Administrate's assets and then build the gem, including the
contents from app/assets/builds in the gem files.

So, the application using administrate won't need to compile those
assets because they are ready-to-use .js and .css files.
The application will be just adding them to its pipeline, generating
digests, and serving the files.

Let me know what you think about this approach!

I'm new to Rails Engine and Administrate, so I may have overlooked something.

@thoughtbot thoughtbot deleted a comment from hound bot Jul 7, 2023
@elias19r elias19r changed the title Attempt at publishing the gem with precompiled assets Attempt at publishing the gem with compiled assets Jul 7, 2023
@nickcharlton
Copy link
Member

This looks like a great start. I think shipping with compiled dependencies is likely my preferred approach, as any other involves a lot of extra work (when releasing, or for end users).

Could you take a start at documenting how this might be for end users? I'd love to see what it's like when integrating for the first time (after that, we'll want to make sure we have a migration guide in place, if it's needed.)

As you've been doing this, have you seen any cases where we can drop some of the JavaScript/CSS dependencies?

//= link administrate/application.js
//= link administrate/application.css
//= link administrate/docs.css
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickcharlton

Would you know why Hound is barking at .js and .rb files? My first time using it and I think we could ignore this one for manifest.js

@elias19r
Copy link
Contributor Author

Hi @nickcharlton

I'd love to see what it's like when integrating for the first time (after that, we'll want to make sure we have a migration guide in place, if it's needed.)

I tested locally today with some new Rails apps and both main and this branch's version seem to work interchangeably, so I think we won't need a migration guide.

Other than that, there is generator called bundle exec rails generate administrate:assets that allows the user to copy the gem's original not-compiled assets over to the application's codebase so they can edit and override the gem's CSS and JavaScript. I haven't tested this yet, but I think as long as we keep the original paths for CSS and JavaScript files, this shouldn't be an issue -- assuming the user's application is properly set up to compile Administrate's assets, otherwise they wouldn't be customizing them in the first place.

I've changed the original CSS and JavaScript file paths in this branch to match the usage of jsbundling and cssbundling (main difference is app/javascript/ instead of app/assets/javascripts). I'll work on undoing that.

As you've been doing this, have you seen any cases where we can drop some of the JavaScript/CSS dependencies?

I haven't seen any cases yet. These are the current dependencies to compile Administrate's assets:

  "dependencies": {
    "esbuild": "^0.18.11",
    "jquery": "^3.7.0",
    "jquery-ujs": "^1.2.3",
    "sass": "^1.63.6",
    "selectize": "^0.12.6"
  },

@jordanstephens
Copy link

I'd be interested in helping get this over the finish line. If you need help, let me know!

@elias19r
Copy link
Contributor Author

elias19r commented Jul 21, 2023

Hi @jordanstephens, thank you!

I think the PR is ready for review. When you get a chance, testing this gem in any Rails apps you have would be helpful!

To build and install the gem locally, I git checkout to this branch and I run this portion of the bin/release script:

VERSION=$(ruby -r ./lib/administrate/version.rb -e "puts Administrate::VERSION")
echo "Version $VERSION"
yarn run build
yarn run build:css
gem build administrate.gemspec
gem install administrate-${VERSION}.gem

(Notice it has the yarn run build and yarn run build:css commands that compile the assets.)

Then, in a Rails app that has administrate set up, I run bundle install and to make sure I'm really using the version I've just built, I run bundle info administrate and then cd into the directory of the gem to inspect it by looking at some files (for example app/assets/builds and app/assets/config/administrate_manifest.js must be present). I smoke test the app by poking around the admin pages.

Then you can build the current version with git checkout v0.19.0 and running the same commands (except for the yarn run commands) to install back the current version and test again in Rails apps with bundle install and running the app. That's to make sure they can be used interchangeably.

When running your Rails app, make sure to clean up any compiled assets with bundle exec rails assets:clobber, removing rm -rf tmp/cache, and maybe rm -rf app/assets/builds/* if your app uses it.

You can also keep the .gem files around and name them like administrate-0.19.0.gem and administrate-0.19.0-with-compiled-assets.gem to make it easier to switch between them with gem install

@elias19r elias19r marked this pull request as ready for review July 21, 2023 16:59
@jordanstephens
Copy link

jordanstephens commented Jul 27, 2023

Hey @elias19r ! Thanks for your work on this. I just wanted to report that I tried out your branch on two rails apps—one using sprockets and one only using import maps—and administrate appears to work as expected in both cases. 🎉 Let me know if there's anything specific I can investigate for you or anything else I can do to help push this effort forward.

@chloerei
Copy link

chloerei commented Aug 13, 2023

I tested it on a demo project using propshaft and it works.

propshaft is a better alternative for cssbundling and jsbundling, hope administrate can push this improvement to adapt to propshaft.

@danielmorrison
Copy link

danielmorrison commented Aug 15, 2023

To add another voice, I'm testing this on the same setup as @chloerei and it works great.

@pablobm pablobm mentioned this pull request Nov 19, 2023
3 tasks
@pablobm
Copy link
Collaborator

pablobm commented Nov 24, 2023

Thank you for this work @elias19r, and apologies that it's taking so long to have it reviewed. I had a look today, and I really want to push this through the finish line.

I've been tinkering and I have my own version at main...pablobm:administrate:publish-gem-with-precompiled-assets-2--digests, which some changes. I still want to test this with some apps, as well as on production-like environments. It's promising so far!

Would you be able to rebase? There have been a few changes since July, so we should get up to date with that.

I changed the package.json scripts a bit. In part to clarify them, but also because the --watch in the CSS command was being applied only to the "internal docs" part, not the "application". This is because it's two commands separated by && and the --watch is just appended at the end, so it only applies to the second command.

I also added bin/dev_assets. I used this while linking my local copy of Administrate from an app in my computer. I wanted to be able to regenerate the files without running the dev server, because my separate app had its own server.

.sample.env Outdated Show resolved Hide resolved
@elias19r
Copy link
Contributor Author

Thank you @pablobm! Sure, I'll rebase it soon

@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
Copy link
Contributor Author

@elias19r elias19r left a comment

Choose a reason for hiding this comment

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

@pablobm I just merged your branch into mine and left comments about the things I didn't pick from you branch. Thank you!

bin/build_gem Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
bin/dev_assets Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Gemfile Show resolved Hide resolved
@elias19r
Copy link
Contributor Author

I'll be removing comments from the Hound bot for now because they are noisy. We can rerun it later

@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 24, 2023
.tool-versions Outdated Show resolved Hide resolved
@thoughtbot thoughtbot deleted a comment from hound bot Nov 28, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 28, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 28, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Nov 28, 2023
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@thoughtbot thoughtbot deleted a comment from hound bot Feb 2, 2024
@nickcharlton nickcharlton merged commit 2a1bf79 into thoughtbot:main Feb 2, 2024
10 checks passed
@nickcharlton
Copy link
Member

…and it's in! Thank you everyone for your work on this, especially @elias19r, who spearheaded this effort and @pablobm too. Plus, everyone who followed along and patiently waited until I finally (!!!) got some time to spend some dedicated weeks working on Administrate.

@jayroh
Copy link
Collaborator

jayroh commented Feb 2, 2024

EXCITING! I can't wait to try this out!

Thank you all (@elias19r, @pablobm, @nickcharlton, and others!) for all of the hard work

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.

8 participants