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

Test with esbuild/propshaft including both Rails 7.2 and 8.0 #3360

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Oct 14, 2024

Adding esbuild to test matrix -- as long as it's with propshaft, it just passes with no code-changes on main in both Rails 7 and Rails 8, because in fact the existing propshaft_generator is really a YarnPackageGenerator -- it works with anything that wants NPM dependencies added with yarn add to a package.json.

Apparently that includes some propshaft setups, but it also includes ESBuild setups.

I think that means it would work in Rails 7 with sprockets and esbuild as well (or any jsbundling-rails option using yarn -- that is, any but bun: esbuild, rollup, or webpack.

Before merging we can discuss at community meeting to make sure there is agreement on the project taking this on.

If we do want to merge this, I would after that try an additional suggested PR that renames PropshaftGenerator to say YarnPackageGenerator; adds explicit options to choose which kind of asset generator you want if the default guesses aren't right (say you have importmaps AND sprockets AND esbuild, it is possible!); and adds another clause to default choice too to make it easier for us to test Rails 7+sprockets+esbuild too.

@jrochkind jrochkind force-pushed the ci_package_json_as_dep branch 2 times, most recently from d04c946 to bccaa04 Compare October 15, 2024 01:05
@jrochkind jrochkind closed this Oct 15, 2024
@jrochkind jrochkind reopened this Oct 15, 2024
@jrochkind jrochkind force-pushed the ci_package_json_as_dep branch 5 times, most recently from 42102a6 to 848d289 Compare October 15, 2024 21:32
@jrochkind
Copy link
Member Author

jrochkind commented Oct 17, 2024

This seemed more significant than something that could be merged with only one committer and one reviewer and I had hoped to talk about it at community meeting, but without one this month I don't want to wait!

I wonder if we could get more feedback here? @jcoyne @cbeer @bess @tpendragon @tampakis @sandbergja please tag in any other interested parties.

The intent here is to support solutions that use Blacklight JS and CSS through the blacklight-frontend NPM package (unlike importmaps for JS; the NPM package is currently being used in some scenarios for CSS I think?).

This requires committing to continuing to maintain and release a working blacklight-frontend NPM package (as we have been doing). This only tests ONE such npm-package-using scenario -- jsbundling-rails with esbuild. I think it's only necessary to test one, if this is supported it should work with any (including vite which I use but don't feel a need to put in CI). The one we test could be jsbundling-rails/rollup instead, that would be fine with me too if anyone prefers it.

this CI shows it passing already, which to some extent is an accident -- it's the PropshaftGenerator that sets up the test app just right for this scenario already. I am not sure what that generator is actually intended for, but it could also be called the YarnPackageGenerator -- it actually works for anything using yarn to add dependencies (including blacklight-frontend) to package.json that will then be used from there. And it depends on the order of "detection" in the AssetGenerator correctly deciding to apply the current PropshaftGenerator in this case.

But I think this shows that supporting this path, that I'd like to support, is actually pretty trivial. Blacklight itself kind of already naturally supports it -- as long as we are okay with continuing to maintain and release a working blacklight-frontend npm package.

The harder part is making sure the generators can automatically create the test apps to test this in CI build. This PR also implies committing to that (until we change our minds), and that can get annoying, as we change logic to be better for other conditions making sure it still can create a test app for this one. That is some maintenance burden.

I express intention to help with that, including possibly suggesting a refactor of some of this generation stuff, possibly to include explicit arguments to the BL generator. Like instead of purely guessing from what's on disk, we could imagine rails g blacklight:install --assets=importmaps or --assets=sprockets (until we get rid of it), or --assets=packagejson or similar things. And setting things up so CI could set an ENV that woudl effect the arguments the installer is run with.

@jcoyne I'm especially interested in your feedback here, what do you think about agreeing to support/CI on an NPM-package-based setup, as this PR would add?

@jrochkind
Copy link
Member Author

Based on feedback from @jcoyne in slack, I'm marking this no longer draft, but definitely still interested in people's feedback and attention -- I consider this accepting that Blacklight now supports npm-package-based blacklight-frontend use, with jsbundling-rails/esbuild being the thing in the CI matrix to test that, and we'll keep it working.

@jrochkind jrochkind marked this pull request as ready for review October 17, 2024 17:17
@jrochkind jrochkind merged commit 47bd00d into main Oct 17, 2024
13 checks passed
@jrochkind jrochkind deleted the ci_package_json_as_dep branch October 17, 2024 17:19
@jrochkind jrochkind restored the ci_package_json_as_dep branch October 17, 2024 17:19
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.

2 participants