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

Rename default manifest.json file to .sprockets-manifest.json #7

Closed
josh opened this issue Mar 17, 2015 · 14 comments
Closed

Rename default manifest.json file to .sprockets-manifest.json #7

josh opened this issue Mar 17, 2015 · 14 comments
Milestone

Comments

@josh
Copy link
Contributor

josh commented Mar 17, 2015

xref rails/sprockets-rails#229

cc @rafaelfranca @juddblair

@josh josh added this to the 3.0 milestone Mar 17, 2015
@juddblair
Copy link
Contributor

Hey @josh I can take a crack at this if it helps.

How do you see supporting both working? We could just look in the output dir for the new filename and fall back on manifest-<whatever>.json if that doesn't exist, and default to the new filename when creating the manifest, or we could have a config variable of some sort I imagine.

My concern with the fallback approach is that it could still cause surprise issues like my manifest.json asset and will be jarring if someone clobbered their assets and rebuilt only to find the manifest filename changed.

Thoughts?

@josh
Copy link
Contributor Author

josh commented Mar 17, 2015

I think the existing manifest.json thing is probably going to be an edge case until its fully deprecated. Though, we can tighten up the regexp for matching this.

I'm thinking, by default,

  1. Look for .sprockets-manifest.json, if it exists use it.
  2. Look for manifest(-[0-9a-f]+)?.json, if it exists use it set some sort of internal legacy flag
  3. Else create a new file called .sprockets-manifest.json

Then on Manifest#save, if its legacy or whatever, write out to .sprockets-manifest.json instead.

I'm wondering if its safe to cleanup any existing manifest-123abc.json files. I'm not sure what people are expecting in there crazy deployment process around this.

@josh
Copy link
Contributor Author

josh commented Mar 17, 2015

Also, I'm wondering if the . is enough to prevent it from being served in all cases. The original reason for adding the random hex was to make the filename unguessable. We may want to still call it .sprockets-manifest-abc123rand.json.

@matthewd
Copy link
Member

I'm nervous about going back to a static name, and trusting the web server's config to refuse to serve hidden files.

@matthewd
Copy link
Member

@josh 😄

Maybe we can make the randomness structurally different from an asset hash, so no matter how improbably someone names their asset, it won't conflict?

@josh
Copy link
Contributor Author

josh commented Mar 17, 2015

Thats true too. Right now the random hex is 32c which is the same length as old MD5 tagged assets. However, were moving to sha256 so that brings it up to 64c. So for legacy manifests, we can at least verify the old 32 length.

@juddblair
Copy link
Contributor

@matthewd - yeah, this a good point. I think it's probably too big an assumption to make re: web server config for little benefit, especially with the naming strategy you mentioned.

@josh - sounds like a good strategy to me. Is the 32 -> 64 move happening with this milestone release? Great timing if so.

@josh
Copy link
Contributor Author

josh commented Mar 18, 2015

2.x defaults to MD5 digests, and 3.x defaults to SHA256.

@elia
Copy link
Contributor

elia commented Mar 18, 2015

Not sure if it's too hard but making it configurable wouldn't make the transition much easier and still possible -for those who want- to use the old naming?

@josh
Copy link
Contributor Author

josh commented Mar 18, 2015

If you specify an explicit manifest path, it always uses that instead of an autogenerated one. If you'd like to use the old name, you can just set it. Thats the way its always worked. The main issue is for people who are using the defaults.

config.assets.manifest = "public/assets/manifest-123abc.json

@elia
Copy link
Contributor

elia commented Mar 18, 2015

Got it. Sorry for the dumb question.

@josh
Copy link
Contributor Author

josh commented Mar 18, 2015

Another thing that might be nice to solve is to make the randness actually deterministic based some secret somehow. I know that sounds contradictory.

Its less ideal that multiple app instances can have different randomly generated manifest paths. One is assets/manifest-123abc.json and other is assets/manifest-456.json.

What if we did a similar thing for how Rails generates csrf tokens and other private secrets and set a environment.secret? Like fresh Rails app's generate an assets.rb initializer like Rails.application.config.assets.secret = "abc123randoperapp". Or just set manifest path directly Rails.application.config.assets.manifest = "public/assets/manifest-123abcrandgeneratedforthisapp.json

I feel like this is the only use case so far for a sprockets secret, but maybe theres others? I dunno. Thats one idea to make it deterministic but still secret from the rest of the world.

@juddblair
Copy link
Contributor

Something like this? master...juddblair:master

I need to fix the glob-style dir stuff to tighten in (was planning on select and regexps), but that should give you the general idea of what I was thinking, plus a test to cover the case of legacy vs new files.

@josh
Copy link
Contributor Author

josh commented Mar 18, 2015

@juddblair yeah, looks like its on the right track!

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

No branches or pull requests

4 participants