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

Hanami::Assets does not preserve directory structure #79

Closed
katafrakt opened this Issue Oct 5, 2017 · 19 comments

Comments

Projects
None yet
5 participants
@katafrakt
Copy link
Contributor

katafrakt commented Oct 5, 2017

I'm creating an app where certain pages are displayed using different themes, based on values from the database. I put theme in apps/assets/stylesheets/themes/orbit/styles.css directory and then referenced it in the template with <%= stylesheet 'themes/orbit/styles' %>. However, this does not work. The stylesheet is not available and when I look into public/assets directory, I see styles.css in the root directory, not in public/assets/themes/orbit/ as I expected.

Is this a bug/missing feature in Hanami::Assets or is it by design? If the latter, how can I accomplish what I want. If former, what can I do to help fix/implement it?

I'm using 1.1.0.beta1 version.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Oct 9, 2017

@katafrakt Hi and thanks for reporting this problem. It's not possible to keep the structure at this time.

We already discussed some time ago (see #34), and my opinion is 👍 for this enhancement. What's @hanami/core take on this?

@jodosha jodosha added the discussion label Oct 9, 2017

@AlfonsoUceda

This comment has been minimized.

Copy link
Member

AlfonsoUceda commented Oct 9, 2017

@jodosha reading #34 I'd like to have this enhancement, namespacing stuff is the best you can do, I mean order your files in subfolders to have your work organized.

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Oct 10, 2017

@katafrakt would you like to implement this?

@AlfonsoUceda

This comment has been minimized.

Copy link
Member

AlfonsoUceda commented Oct 10, 2017

@katafrakt if you can't I could give it a try

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Oct 10, 2017

I would like to and I'll try to do it.

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Oct 11, 2017

@jodosha Some questions before I actually code:

  • Should the PR target master or develop?
  • Should there be an option to retain old (current) behaviour? Since Hanami is past 1.0, it might be a good idea, although it will make the code more complex, probably.
@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Oct 16, 2017

@katafrakt

Should the PR target master or develop?

Please target develop

Should there be an option to retain old (current) behaviour? Since Hanami is past 1.0, it might be a good idea, although it will make the code more complex, probably.

As long it doesn't require changes in the public API, we can include this change in the next 1.x release. I see compiled assets not as a breaking change, as long they will keep working for people. The directory structure should be irrelevant for devs.

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Oct 17, 2017

As long it doesn't require changes in the public API, we can include this change in the next 1.x release.

It's really hard for me to tell. Basically the problem I see is that right now there is a mechanism resolving assets from nested paths, i.e. if I have a file in assets/some/directory/bootstrap.js and do Hanami::Assets::Compiler.compile(@config, 'bootstrap.js'), it will be found and copied into the root directory of assets as bootstrap.js. I think this should be removed and only "full relative paths" should be supported. But that sounds like a breaking change. What do you think?

(if you are against removing it, the PR is really simple and I can do it right away, btw)

@Titinux

This comment has been minimized.

Copy link

Titinux commented Oct 20, 2017

This is what I am thinking about :

1 - Assets that don't need processing.

Have a list of mime types that pass through assets manager without being modified (jpg, png, woff, svg...)

Example :
assets/images/my-theme/backgrounds/bg_1.jpg => compute hash if needed => public/assets/images/my-theme/backgrounds/bg_1-hash.jpg

2 - Assets that need compilation/preprocessing/whatever

Have a list of assets to release

Example :
assets/stylesheets/my-theme/blue.sass => use sass preprocessor to include dependencies and produce css file => compute hash if needed => public/assets/stylesheets/my-theme/blue-hash.css

Simple, predictable and still configurable.

Am I missing something ?

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Oct 26, 2017

Since this issue did not make it to 1.1.0 release, I would like to propose a bit more "radical" change instead of what's in my pull request.

Right now every asset from root is compiled to target directory no matter how deep it is in the file structure. This means the following results (assume root is web/assets and target directory is public/assets):

path compiled to
web/assets/style.css public/assets/style.css
web/assets/stylesheets/style.css public/assets/style.css
web/assets/foo/bar/style.css public/assets/style.css

The PR mentioned in this issue add an opt-in possibility to mark some paths as nested so they are compiled differently. For example with this added to config:

nested_assets << [
   "style/style.css"
]

web/assets/stylesheets/style/style.css would be compiled to public/assets/style/style.css.

This solution is backwards compatilble, but in my opinion it is not the best one. Instead, I would like to propose creating a list of directories, where files can be looked up under root. If the file is not in one of them or in nested in a deeper structure, it is treated as relative path.

Assuming that "allowed directories" would be stylesheets, javascripts, images and fonts it would look like that:

path compiled to
web/assets/style.css public/assets/style.css
web/assets/stylesheets/style.css public/assets/style.css
web/assets/foo/bar/style.css public/assets/foo/bar/style.css
web/assets/javascripts/bootstrap/main.js public/assets/bootstrap/main.js

I don't know if I described it clearly, but let me know what you think.

@AlfonsoUceda

This comment has been minimized.

Copy link
Member

AlfonsoUceda commented Nov 11, 2017

ping @jodosha

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Jan 8, 2018

any news?

@cllns

This comment has been minimized.

Copy link
Member

cllns commented Jan 8, 2018

I think it's OK if it's not backwards compatible, since the old behavior was buggy.
We should strive for correctness (preserving hierarchy) over keeping compatibility with buggy behavior.

@cllns

This comment has been minimized.

Copy link
Member

cllns commented Feb 11, 2018

@katafrakt I just ran into an effect from this bug. I'm building an app using Semantic UI, and the CSS included looks for a font at ./themes/default/assets/fonts/icons.woff2 but since we don't nest assets, it cannot find it. By editing it to just ./icons.woff2, it works. So, I really think this bug fix would be great :)

Also, I personally think we shouldn't white-list folders (stylesheets, javascripts, images and fonts). We don't do it now, and we'd want to support any arbitrary file structure within assets. What do you think?

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Feb 18, 2018

@cllns Perhaps I did not explain it correctly but there is no real "whitelisting" in my concept, more like adding additional root directories for assets. For example:

apps/web/assets/themes/default/assets/fonts/icons.woff2 becomes public/assets/themes/default/assets/fonts/icons.woff2

however apps/web/assets/stylesheets/semantic/semantic.css would become public/assets/semantic/semantic.css and not public/assets/stylesheets/semantic/semantic.css.

stylesheets, javascripts, images and fonts would be those additional roots.

@cllns

This comment has been minimized.

Copy link
Member

cllns commented Feb 21, 2018

@katafrakt Ah, I think I get it now. Thanks! Do you think you'll have time to open a PR implementing this fix, against develop?

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Feb 21, 2018

@cllns I hope so, but I will take couple of days. I have some work in progress already though.

@katafrakt

This comment has been minimized.

Copy link
Contributor

katafrakt commented Mar 7, 2018

@cllns There is a new PR (linked above)

@jodosha jodosha added this to the v1.3.0 milestone Aug 10, 2018

@jodosha jodosha self-assigned this Aug 10, 2018

@jodosha

This comment has been minimized.

Copy link
Member

jodosha commented Aug 10, 2018

Implemented by #88

@jodosha jodosha closed this Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment