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.

Sign up for GitHub

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

Jump to bottom

Put prod assets in version control #106

Merged
merged 1 commit into from Aug 30, 2018
Merged

Put prod assets in version control #106

merged 1 commit into from Aug 30, 2018

Conversation

MusikAnimal
Copy link
Member

This is recommended by Symfony, and also will get around the
Node issues we are running into on Toolforge.

See https://symfony.com/doc/current/frontend/encore/faq.html#do-i-need-to-install-node-js-on-my-production-server

Bug: https://phabricator.wikimedia.org/T202471

@MusikAnimal
Put prod assets in version control
2850753 
This is recommended by Symfony, and also will get around the
Node issues we are running into on Toolforge.

Bug: https://phabricator.wikimedia.org/T202471
@aezell
Copy link

aezell commented Aug 29, 2018

What triggers the rebuilding of the asset files? Are they built on every build in Travis (or whatever)? If so, how are the filenames generated? Do the filenames change if the contents of the file hasn't changed?

I'm wondering what happens if you commit certain filenames and then another engineer commits a different set of filenames. There'll potentially be a git merge conflict but maybe the file contents haven't even changed.

@MusikAnimal
Copy link
Member Author

Development is done in app/Resources/assets. You specify the files in webpack.config.js. Then when the ./node_modules/.bin/encore production command is ran it compiles and concatenates them into app.js and app.css. These two files are versioned automatically, and sourced with {{ asset('assets/app.js') }} in the <head>. So there could be filename conflicts but the same was true with Assetic.

I guess we have to remember to compile the production assets in our pull requests, but similarly we had to remember to bump the asset version when we were using Assetic. Travis doesn't care about assets because the integration tests use a headless browser. That being said it would be nice if only the deploy script compiled production assets, and we didn't have to commit them, but we shall bow to convention (and we have Node issues on Toolforge anyway).

@MusikAnimal
Copy link
Member Author

I guess we have to remember to compile the production assets in our pull requests

Actually this I guess isn't a problem, so long as we always compile as production and not dev when working locally (since you'll obviously want to compile assets one way or another in order to see your changes during development). Your browser (at least Chrome, Firefox, & Safari) has an un-minify feature, so you could still easily add in JavaScript breakpoints when debugging.

@aezell
Copy link

aezell commented Aug 30, 2018

That sounds good. Thanks for the explanation!

I've definitely done many a commit that just said "version bump."

samwilson
samwilson approved these changes Aug 30, 2018
View reviewed changes
Copy link
Member

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Should we set up a pre-commit hook that compiles the assets?

@samwilson samwilson merged commit 20c6047 into master Aug 30, 2018
@samwilson samwilson deleted the commit-prod-assets branch August 30, 2018 01:22
@aezell
Copy link

aezell commented Aug 30, 2018

I like the idea of the pre-commit hook.

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

@samwilson samwilson samwilson approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@MusikAnimal @aezell @samwilson