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

Use sprockets 3/4 style manifest.js #1759

Merged
merged 1 commit into from
Mar 14, 2017

Conversation

jhawthorn
Copy link
Contributor

Sprockets 4 is coming along and is currently 4.0.0.beta4. They have a nice upgrade doc, and some really great features (source maps, built-in babel+es6).

One issue is that sprockets 4 doesn't seem to support "wildcards" (like spree/backend/all*) in the config.assets.precompile list. It wasn't obvious, but it's clear this removal was intentional. The behaviour was listed in sprockets 3.x as deprecated and was in legacy.rb.

The sprockets upgrade notes push for using a app/assets/config/manifest.js instead of loading files into the precompile array, which seems reasonable. We can do the same, but we need a unique name so we don't collide with

In lieu of wildcards, we can use link_tree in most cases. This handles images cleanly.

The obvious ugliness is that we need to explicitly reference all our select2 locale files. I don't think there's a way around this while still using the select2-rails gem. We can't use link_tree because the relative path is unknown (in a gem). One solution would be to embed select2-rails in the vendor/assets directory (which we might want to do anyways).

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I think vendoring select2 is a good idea. Maybe someone wants to use a different version of this gem in their app and can't right now.

Also we could add a way to let people decide which locales of select2 they import. A solidus-backend-select2.js file that can be overridden by the host app for instance.

Not sure if we should tackle this in this PR

Otherwise: Yay sprockets 4 🎉

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

I'm also pro moving off of select2-rails gem and vendoring it.

@jhawthorn
Copy link
Contributor Author

I'll vendor select2 in a separate PR

@jhawthorn jhawthorn merged commit 057fb01 into solidusio:master Mar 14, 2017
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.

3 participants