pinax / pinax-starter-projects

Pinax Starter Projects
MIT License
82 stars 39 forks source link

Use static templatetag to remove hardcoded paths #30

Closed tubaman closed 7 years ago

tubaman commented 8 years ago

These hardcoded paths cause issues. For example, if I want to host the site at a different root path instead of '/', I have to edit these.

Also, using the static template tag in the handlebars source allows me to move the site around without having to install node/npm/gulp and rebuild the assets.

This is much better than my last attempt because I actually made the change in the handlebars source rather than the generated files.

Also, I realized that we get some DRY here by being able to remove staticUrlRoot from the handlebars config.

ossanna16 commented 8 years ago

Thanks @tubaman! @jtauber @paltman Can this be merged?

paltman commented 8 years ago

No.

These are source handlebars templates and are not processed by Django. They are part of the gulp static build system.

paltman commented 8 years ago

Reopening because i just reread the PR description and it seems you were aware of this but doing something I hadn't expected. Let me think about this.

tubaman commented 8 years ago

On Tue, Apr 26, 2016 at 10:40:37AM -0700, Patrick Altman wrote:

Reopening because i just reread the PR description and it seems you were aware of this but doing something I hadn't expected. Let me think about this.

Ok, let me know if you'd like to discuss on email/IRC/whatever.

ossanna16 commented 8 years ago

@tubaman It would be great to discuss this in our Pinax Slack, if you don't mind joining (http://slack.pinaxproject.com). Thank you! :)

paltman commented 8 years ago

I'd prefer to have the discussion here for documentation purposes as we get a lot of questions about the static process so having a history of conversations related to PRs, I think is better preserved here.

paltman commented 8 years ago

@tubaman The most immediate problem I have with {% static %} in the static/src/* files is that those files are not processed by Django at all. It was a intentional decision to separate the static build process from Django so that things can be more decoupled.

I am assuming you have tested this and the {% static bit gets ignored by the handlebars processing? I would think that would cause an error.

tubaman commented 8 years ago

On Wed, Apr 27, 2016 at 05:57:58AM -0700, Patrick Altman wrote:

@tubaman The most immediate problem I have with {% static %} in the static/src/* files is that those files are not processed by Django at all. It was a intentional decision to separate the static build process from Django so that things can be more decoupled.

I totally agree that the static static build should be separate from Django so that folks like me don't have to install npm/gulp/etc to use the starter projects.

However, there are touch points between Django and the static assets. For example:

  1. Where the static assets are located(STATIC_URL in settings)
  2. Where the assets are served from(file system, CDN, etc)

Django has really nice facilities to enable us to easily manage all that. Pinax should take full advantage of it.

In this discussion I realized that part of my change didn't make it into the PR so I've fixed up the PR to include that change. Namely, we don't need staticUrlRoot in the gulp config anymore since it's already defined in settings.py we're using the static templatetag now. DRY FTW!

I think this whole discussion boils down to this: Which do we care about more, decoupling or DRY?

I am assuming you have tested this and the {% static bit gets ignored by the handlebars processing? I would think that would cause an error.

Yup, I've tested it but running npm run build and the _scripts.html and _styles.html got generated correctly. I've added a new commit to the PR which is the result of me running npm run build.

Since handlebars runs first before the Django template is processed, it works fine since "{% ... %}" isn't a handlebars expression. Also the only "{{ ... }}" expression in those files is meant for handlebars, not Django.

Aside: If there were a "{{ ... }}" expression meant for Django, we'd have to figure out a way to escape those curly braces so handlebars doesn't process them by mistake. However, for now we don't have that problem.

Thanks for all the work. Regardless of which way this goes, Pinax is hugely useful to me.

tubaman commented 8 years ago

Any further thoughts on this?

ossanna16 commented 8 years ago

@paltman ^

tubaman commented 8 years ago

Just checking in. Any thoughts on this?

brosner commented 8 years ago

I came across this independently while optimizing static assets on a project. I am in favor of this change, but I wonder if we could do more. The hashing / manifest during the gulp build might be better served by Django's staticfiles:

https://docs.djangoproject.com/en/1.9/ref/contrib/staticfiles/#manifeststaticfilesstorage

This will hash every file for proper caching by CDNs and browsers. This may be better served by some Node package integrated into the pipeline, but no solution has been presented for it yet. I'm open to that.

I think this PR is a good first step. I will make some comments in the code to get this ready for merge.

tubaman commented 8 years ago

@brosner I also like the idea of handling static assets in Python. It just seems like the tools are often weaker than those available in javascript.

tubaman commented 8 years ago

Just a note. Since we're using less, we'll always need some tool to compile that into css. I think the right thing is to have some of the pipeline in javascript(node, gulp) and some in python(django, etc). The question is: what parts of the pipeline should be in javascript and what parts are in python?

paltman commented 7 years ago

I'm am working on something that I think will address this and borrow heavily from the ideas here though it make make it hard to just merge this as well. Either way, I appreciate your contributions @tubaman, and I've come around to your way of thinking. :)

paltman commented 7 years ago

So in my effort to solve #45 I ended up simplifying things so there is no longer a need to have includes generated at all since per @brosner's recommendation we just let Django do the hashing. I originally avoided this idea because it used to be the case you had to add a third party library and do some configuration to get this hashing feature (the name of this popular library is slipping my memory right now). It's cool to see it added to Django and has sane defaults so that just a single line in the settings.py gets it working.

The work I have done for #45 can be found in this compare: https://github.com/paltman/zero-gulp-removal/compare/c277f0536bc5d3a1a21fb41bc3647e55ecb182fd...master and I hope to get this backported to pinax projects before the end of the month (before our 16.10 distribution).

paltman commented 7 years ago

@brosner would love your thoughts on the above.

@florapdx likewise, i'd love your thoughts on the package.json scripts.

paltman commented 7 years ago

@tubaman Just pushed everything. After some testing we will create releases. For now you can test your self any pinax project by using the -dev flag on the pinax CLI:

pinax start --dev account <PROJECTNAME>
tubaman commented 7 years ago

Glad to be a part of the solution. Thanks @paltman and @brosner