josefarias / hotwire_combobox

An accessible autocomplete for Ruby on Rails.
https://hotwirecombobox.com
MIT License
499 stars 34 forks source link

Should we support all asset pipeline options? #10

Closed josefarias closed 8 months ago

josefarias commented 9 months ago

I know importmaps are well supported. Sprockets and propshaft need some work. I haven't tested webpack at all.

pranavbabu commented 9 months ago

It seems esbuild will not work , due to main js source is taken from app/javascript. And here probably best answer how to fix it

josefarias commented 9 months ago

@pranavbabu thanks! Do you have a use case where it's imperative we publish to npm? Or where the current setup doesn't work out of the box?

I think many (most?) apps that use jsbundling with esbuild, bun, etc would use sprockets for css.

Because this library already precompiles its JS in the manifest, sprockets should take care of importing it. Even if its intended purpose is to handle css.

The exception here might be webpack. Because it can also bundle css, I'm sure many apps used it to replace sprockets entirely. And so this gem would not work out of the box because its automatic methods of importing JS only work for importmaps and sprockets. But then those apps are probably using a front-end framework like react and probably wouldn't reach for this gem. And can simply add the importmaps gem if they really wanted to.

Would love some input from the community here. I'd like to avoid publishing to npm unless enough people have a need for it.

sevab commented 9 months ago

Sharing my +1 vote for NPM, but my use-case is non-standard outside of Rails (I use Hotwire with Elixir).

josefarias commented 9 months ago

Thanks for sharing that use case @sevab!

A large part of this gem is the affordances it brings due to being a Rails engine. It's a large part of why it's easy to use.

The JS alone can't provide as smooth an experience. So that's one reason I don't want to publish it to npm — it's not meant to be a standalone JS library. Not today, anyway. I'm not opposed to a future where it is, but the initiative would most likely not come from me personally.

asecondwill commented 8 months ago

I've switched our apps to propshaft & importmaps with dartsass. will it work with that setup? it seems that will be the default eventually for rails (minus dartsass but i'm not letting go yet)

josefarias commented 8 months ago

I've switched our apps to propshaft & importmaps with dartsass. will it work with that setup? it seems that will be the default eventually for rails (minus dartsass but i'm not letting go yet)

@asecondwill it should! Please reach out if it doesn't (with as much information as possible) and I'll get it sorted.

Any setups that include importmaps and/or sprockets should work out of the box just by installing the gem.

pranavbabu commented 8 months ago

You are correct about css, but I'm talking about having bundled js code like controller and other logic you use, which is not possible ATM. Thing is I have <%= javascript_include_tag *all_javascript_bundles, "data-turbo-track": "reload", defer: true %> and

def all_javascript_bundles
    ['application']
  end

which makes impossible to link all js files that sprockets generates for your gem, unfortunately.

josefarias commented 8 months ago

@pranavbabu Ah you're right, we're missing a helper like the one we have for including stylesheets. I thought we had one! I'll add one today and include in the readme.

EDIT: Ran out of time to implement this week. I'll come back to it later. Would also welcome a PR for this, I think we'll have to bundle all the JS for inclusion. And probably separate the css and js manifest so they can be required separately.

joshfester commented 8 months ago

This would solve a major pain point for me. Most of my apps are using propshaft, jsbundling-rails, and esbuild. Would that be supported?

josefarias commented 8 months ago

I had initially forgotten to mention Propshaft. As things are, we need additional helpers and config to handle JS with Sprockets and Propshaft. Definitely want to support both.

I'm not programming for a couple of weeks, but I will be reviewing on my phone. So a PR for this would be very welcome. Also happy to tackle this when I'm back.

KonnorRogers commented 8 months ago

There's actually multiple ways to handle this.

for Rails + ESBuild users with the gem installed, you can add a "node_path" to ESBuild to search within the "hotwire_combobox" gem location.

https://esbuild.github.io/api/#node-paths

the other option for those either who don't want to use gem lookup paths or are not using Rails (unsupported I know, but we all commit crimes here right?) is package.json supports downloading files directly from git sources.

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#github-urls

EDIT: as I'm poking through the source code because imports are done using "bare specifiers" you'll probably need to add NODE_PATH lookup if you install from Git

josefarias commented 8 months ago

Gave this another go today and I gotta say I'm stuck. Would appreciate some help here.

Here's why things currently work with importmaps:

  1. The engine defines its own importmap where all JS files are pinned.
  2. This importmap is included in config.importmap.paths via a railtie initializer.
  3. The host application (i.e. the user's app) renders the importmap script tag via javascript_importmap_tags.
  4. The presence of the engine's single stimulus controller in the importmap script makes it visible to stimulus-loading inside stimulus-rails.
  5. As long as the host app makes use of one of the stimulus-loading methods (e.g. eagerLoadControllersFrom) for loading its own controllers, then the app also loads the combobox stimulus controller and we're all set.

I really like this because it requires zero configuration on the user's end. I'd like whatever solution we end up with for other pipeline backends to be as zero-config as possible.

Now, the engine already adds its assets (CSS and JS) to config.assets.precompile. This was mainly done to support loading the provided CSS via sprockets. I think that's enough to also make the JS available to any app using propshaft or sprockets. But... I'm not sure what should happen next.

We still need to import the stimulus controller and add it to the host window.Stimulus application. And we need to do that as a module so that all the inner import statements in the combobox controller work.

One thing we could do is bundle all the JS and upload it to npm. Host apps could then download it as a node_module, require the controller in their JS, and register it in their window.Stimulus. Not zero-config but not bad either. But I really don't like having two places to deploy every release. I also don't like that the JS alone doesn't give you much of anything so it feels weird to publish by itself. Still, if this is our only choice, I can live with it.

But it feels like we should be able to sidestep npm being that all the assets are available through the gem already. Any ideas how we could do this?

Finally, I wonder if it'd be so bad to just require importmaps as a hard dependency. For any non-importmaps users, we could just instruct them to use the stimulus-loading helpers. They don't even have to know they're using importmaps behind the scenes and things will just work with minimal config.

I admittedly don't often tinker with the asset pipeline's plumbing. Feels like every time I do I have to re-learn everything 😅. Appreciate y'alls help on this.

@pranavbabu, @KonnorRogers, @excid3, et al. hate to cc you on this wall of text but I'm hoping you could share your impressions given that you've expressed interest in using this with esbuild and propshaft.

excid3 commented 8 months ago

The nodePaths option might work for esbuild, but you need an easy way of getting that path from your node script. I guess we could shell out to bundle show hotwire_combobox to try and get the gem path?

josefarias commented 8 months ago

But that's just esbuild right? Or would that work for other bundlers as well?

If it's the latter then I might've been overcomplicating things and we can just use that everywhere.

Shelling out seems acceptable to me.

josefarias commented 8 months ago

Seems like NODE_PATH works on esbuild because they manually replicated its functionality.

I couldn't find any official deprecation notice but it seems like the node community in general doesn't recommend using NODE_PATH. Found this quote from a 2015 (old) TC meeting:

In 2.x, probably warn on any use of NODE_PATH at all

Looks like NODE_PATH doesn't work with rollup at all. I wonder if they need to implement their own version manually, like esbuild did.

I'd like to rely on a more universal foundation so we can use the same approach everywhere. I think that means unless someone knows how to require the stimulus controller from the precompiled assets in the gem then we need to publish to npm.

Oh, well. Guess I'll get started on that.

KonnorRogers commented 8 months ago

@josefarias Rollup is pretty bare bones, although if you're using NPM, chances are you're using nodeResolve

The nodeResolve plugin supports: modulePaths which is their version of NODE_PATH

https://www.npmjs.com/package/@rollup/plugin-node-resolve#modulepaths

For Vite, you're probably looking at setting a resolve.alias https://vitejs.dev/config/shared-options.html#resolve-alias

josefarias commented 8 months ago

@KonnorRogers so would that mean providing instructions for each bundler with something akin to this?

{
  "name": "app",
  "private": true,
  "dependencies": {
    "@hotwired/stimulus": "^3.2.2",
    "@hotwired/turbo-rails": "^8.0.3",
    "esbuild": "^0.20.1"
  },
  "scripts": {
    "build": "NODE_PATH=$(bundle show hotwire_combobox) esbuild app/javascript/*.* --bundle --sourcemap --format=esm --outdir=app/assets/builds --public-path=/assets"
  }
}

(using package.json for brevity – but we'd provide examples for bundler config files, too)

I tried the above and esbuild can't seem to resolve the internal import statements in the stimulus controller, although it does load the stimulus controller at the very least.

I'm thinking let's just publish to npm. Everyone should know how to make that work for their setups and if we're going to require some config then I'd like us to stick to the norm here. Otherwise we risk getting into hairy situations on deployment or strange local setups.

josefarias commented 8 months ago

Here's a PR where I've set up bundling for publishing on npm. Would love some eyes on it if anyone has a spare minute. Thanks!

josefarias commented 8 months ago

The package is up on npm now so if anyone following this wants to give it a go they can follow the instructions in https://github.com/josefarias/hotwire_combobox/pull/69

josefarias commented 8 months ago

The two PRs are merged. And with that, we now support all JS backends for the asset pipeline, including esbuild, rollup, webpack, etc.

No release is needed. The readme is updated with the instructions and that's enough to run the current version which is v0.1.37.