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

Allow using a custom assets configuration manifest #446

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kennyadsl
Copy link

@kennyadsl kennyadsl commented Oct 10, 2019

Ref #369

Using Sprockets 4, at the moment, the only allowed path for the newly required configuration manifest is the default one (app/assets/config/manifest.js) and there is no way to change it.

This PR adds a way to change the default location by adding a new config.assets.config_manifest option, that could be set like this:

config.assets.config_manifest = File.expand_path('my_custom_path/config/manifest.js', __dir__)

Additional context

In Solidus, we use a custom dummy app to perform our specs. We need to configure the path with custom locations for the configurations files we use. This PR allows us to set a custom configuration manifest path, otherwise, we would not be able to be compatible with Sprockets 4.

Here are some relevant parts of our DummyApp, I hope it helps to contextualize our issue.

And this is the failed build output (only first lines are relevant) now that sprockets 4 is used.


This is my first contribution to this project so feel free to point out if I missed something important or how can I improve the PR. Thanks!

@ahorek
Copy link

ahorek commented Oct 10, 2019

here's an old discussion about this #369 I really support to make this optional!

a few notes...
the error message has to fixed

msg = "Expected to find a manifest file in `app/assets/config/manifest.js`\n" +

and there's a failing test
def test_manifest_path

@kennyadsl kennyadsl force-pushed the kennyadsl/fix-config-assets-manifest branch 3 times, most recently from cab84c0 to 1183308 Compare October 10, 2019 13:09
@kennyadsl
Copy link
Author

Thanks, @ahorek! Not sure failure is related now.

lib/sprockets/railtie.rb Outdated Show resolved Hide resolved
lib/sprockets/railtie.rb Outdated Show resolved Hide resolved
@kennyadsl
Copy link
Author

Looking twice, I don't think this is a good change since, if I got it correctly, the config.assets.manifest configuration was used as the location of the output manifest (.yml) after the precompile (searching for config.assets.manifest here).

Even if this is a somehow accepted solution, I think we should use another option to avoid messing with the old purpose of this configuration. Maybe something like config.assets.configuration_manifest?

Copy link

@elia elia left a comment

Choose a reason for hiding this comment

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

👍 FWIW, just one small suggestion for the spec

Comment on lines 299 to 310
Dir.chdir(app.root) do
dir = "app/assets/config/foo"
FileUtils.mkdir_p(dir)
File.open("#{ dir }/bar.json", "w") do |f|
f << ""
end
end
Copy link

Choose a reason for hiding this comment

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

Suggested change
Dir.chdir(app.root) do
dir = "app/assets/config/foo"
FileUtils.mkdir_p(dir)
File.open("#{ dir }/bar.json", "w") do |f|
f << ""
end
end
app.root.join("app/assets/config/foo/bar.json").tap { |path| path.dirname.mkpath }.write('')

Copy link
Author

@kennyadsl kennyadsl Oct 10, 2019

Choose a reason for hiding this comment

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

@elia I preferred to use the same format to create the file as already used on top of this document, but I'm open to change it. Let's see what other maintainers think about this.

@rafaelfranca
Copy link
Member

We need a different config for this. The assets.manifest is the location of the output manifest. Maybe config.assets.config_manifest?

@kennyadsl kennyadsl force-pushed the kennyadsl/fix-config-assets-manifest branch from 12f3382 to 53ae935 Compare October 11, 2019 13:24
@kennyadsl kennyadsl changed the title Allow using a custom assets manifest via app config Allow using a custom assets configuration manifest Oct 11, 2019
@kennyadsl
Copy link
Author

@rafaelfranca added a new config named config_manifest for this. Let me know if this works!

@kennyadsl
Copy link
Author

Another improvement (maybe with another PR) could be falling back to the Sprocket 3 behavior (app.config.assets.precompile += [LOOSE_APP_ASSETS, /(?:\/|\\|\A)application\.(css|js)$/]) if the configuration manifest does not exist. Do we really need to raise an exception if the configuration manifest is not present in the host application? Thoughts?

@rafaelfranca
Copy link
Member

Yes, we need to raise an exception. LOOSE_APP_ASSETS is a proc and procs are not supported anymore in the precompile option.

initializer :set_config_manifest_path do |app|
config_manifest_path = Pathname.new(
app.config.assets.config_manifest ||
::Rails.root.join("app/assets/config/manifest.js")
Copy link
Member

Choose a reason for hiding this comment

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

Let's set this default value on line 135.

Copy link
Author

@kennyadsl kennyadsl Oct 14, 2019

Choose a reason for hiding this comment

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

@rafaelfranca sorry, I'm not sure to get this request. Do you mean the whole set_config_manifest_path initializer or just the default value of the app.config.assets.config_manifest configuration?

Setting the whole initializer at line 135 is not working since when used in the set_default_precompile initializer (before line 135) it is raising an exception since it's waiting for a string but it gets nil:

Error:
TestRailtie#test_task_precompile:
NoMethodError: undefined method `start_with?' for nil:NilClass

Copy link
Member

Choose a reason for hiding this comment

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

I mean setting ::Rails.root.join("app/assets/config/manifest.js") (the default value when the config is not set) and removing this || from this intializer.

Copy link
Author

@kennyadsl kennyadsl Oct 17, 2019

Choose a reason for hiding this comment

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

I don't think it works this way. The Rails app has not yet been initialized when the code at that level is evaluated.

I'm doing this:

    # ...
    config.assets.manifest    = nil
    config.assets.quiet       = false
    config.assets.config_manifest = ::Rails.root.join("app/assets/config/manifest.js")

    initializer :set_config_manifest_path do |app|
      config_manifest_path = Pathname.new(app.config.assets.config_manifest)
      raise ManifestNeededError.new(config_manifest_path) unless config_manifest_path.exist?
      app.config.assets.config_manifest = config_manifest_path.to_s
    end

but Rails.root does not seem to be ready yet at that point; I get this exception:

undefined method `join' for nil:NilClass (NoMethodError)

I think that to use ::Rails.root we need to call it from an initializer's context, right?

At the moment the only allowed path is the default one but
there could be the need to set a custom configuration manifest
location.

This commit introduces a new setting `config.assets.config_manifest`
that allows to change the location of this manifest.
This is an example configuration:

```
config.assets.config_manifest = File.expand_path('my_custom_path/manifest.js', __dir__)
```
@kennyadsl kennyadsl force-pushed the kennyadsl/fix-config-assets-manifest branch from 53ae935 to 8b87902 Compare October 17, 2019 07:35
@aldesantis
Copy link

@rafaelfranca any updates on this by any chance? It'd be really great to have it! 🙏

@kennyadsl
Copy link
Author

@rafaelfranca hey there, is there any thing I can do to move this forward?

@kennyadsl
Copy link
Author

@rafaelfranca friendly ping if you have time to point me in a good direction to move things forward here. Thanks!

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.

5 participants