craigspaeth/nap

About the generated gitignore file

Closed this issue · 18 comments

madhandennis commented

Hi Craig,

Just wanted to find out why a .gitignore is getting added during production mode. I did check the issue list and I came across #25. However this makes it harder to package compressed assets. My situation is as follows:

I have a huge set of asset packages configured and all work fine. Now currently I am packaging assets when NodeJS is started with production mode. Now what happens is that whenever I restart my app, I need to wait for quite a while tile my assets are packaged and built. One alternative is to use one of the libraries available to handle no downtime restarts. Another option would be to do a so called "asset building" step during my deployment flow. So I now have a small CLI which will help rebuild my assets folder (basically calling nap.package() after configuring the nap library) and I want to check in my build assets into my version control since my deployment is a Git based deployment system as well.

So I was wondering what was the reasoning behind having a gitignore in the public assets folder or what negative impact would the above deployment process have?

Thanks :)

madhandennis commented

Hmmm scratch that. I am not able to run it in production mode without calling package first. Is there any way I can pre-package assets before running the web server?

craigspaeth commented

I don't mind removing the automatic writing of the .gitignore file in the assets directory. We use CDNs at Art.sy so it didn't occur to me that one might want to deploy assets by checking them into git. (Let me know if this solves your problem and I'll push the fix).

As for your second question, nap.package can take a callback as an argument after it's finished compiling/minifying/gzipping so if you want to start your webserver after your assets are packaged you can do so rather easily. (This needs to be documented, thanks for pointing this out).

var app = require('express')(),
    nap = require('nap');

nap({ assets: {} });

nap.package(function(){
  app.listen(3000);
});

Although I'm not sure I would recommend this because it'll probably mean that your web server will be down while nap packages assets.

madhandennis commented

Hi Craig,

Actually I do not want to go the route you have written w.r.t the second point. This is my ideal flow:

  1. Make changes to the assets
  2. Have a cli (using commander.js or something) generate the packaged assets
  3. Commit both the source and generated assets into Git
  4. Deploy the app to the servers (using Git)
  5. Restart the web servers
  6. Be able to use the JADE helpers (nap.js, nap.css, etc) to serve my packaged assets without having to .package() them again

The problem currently is that when Nap is put into production mode (which I guess is needed for it to serve the generated files since I use the JADE helpers), it requires me to call package() if its in production mode:

nap.package() must be called before nap can be used in production mode from line 411: throw new Error("nap.package() must be called before nap can be used in production mode") if pkgContents == undefined

To elaborate a little more, my deployment consists of multiple servers running NodeJS and Nginx. I use Nginx on production mode to serve the packaged assets. So my Nap asset packages are served via Nginx and I use Nap's CDN URL to point to the packages inside my app's git repo.

So from the code my issue would be that the fingerprintCache is actually filled during my build/cli stage currently and is empty during my web server startup stage since I don't want to call package there.

Does my idea make sense or am I doing something majorly messed up here?

craigspaeth commented

Makes sense to me, I see what the issue is. Seems to be in conflict with another use case: #40

Committing both assets into git is a simple matter of not adding a .gitignore automatically and forcing users of nap to .gitignore their /public/assets directory themselves if they choose. I'm fine with that so I'll commit that fix.

As for the .package trouble I'll take a deeper look into what the problem for @henrit was and see if I can find a solution that doesn't require nap.package() to be called for fingerprints.

Thanks for using my project and helping me through this!

madhandennis commented

Ah thanks for linking that issue. I am guessing that @henrit and my use case are a bit different. The use case for #40 seems to be more towards using packaged assets without having an external CDN or a web server (like me) serving the final assets. Hence the issue with stylus, etc since it would involve on the fly changes, etc.

Maybe adding a new flag in the options would be a good idea. Something like external_serving or something which would override all the checks and only generate the finger prints and URLs and leave it to the user to ensure that the files really do exist when the browser requests for them?

henrit commented

The issue with #40 boils down to fingerprinting being done on the source assets rather than the generated assets.
Because of stylus' @import statement (at least. There could be other similar scenarios with any other js/css preprocessor that allows including other files), it is possible for generated assets to differ without the source assets changing.
Since I was using a CDN, with all the aggressive caching that entails, I ended up serving stale files on my site in spite of having seemingly done everything right.

So the fix I offered prepares the generated assets before computing the fingerprints in every case where fingerprints are needed. That guarantees truly unique fingerprints and avoids stale CDN collision issues.

Except, as you noticed, generating assets on every production server restart can get to be an expensive proposition.

I'd still recommend to keep calculating fingerprints from generated assets. However, it could make a great deal of sense to allow nap.js to bootstrap itself from a pre-existing fingerprint file, that nap.js/nap.css calls would use to insert proper URLs to the nap-managed assets.

So we'd be looking at:

  • having .package() serialize its fingerprints to a file.
  • having a new method ( .loadFingerprints() ?) that can be called in lieu of .package(), that will assume everything is already correctly packaged and proceed from there.

Would that make sense? We'd keep the naive use case of calling .package() in prod and having things guaranteed to work, and we'd allow using .package() at build time, and .loadFingerprints() at runtime for more advanced scenarios.

henrit commented

Something like this: henrit@296b5bf

This doesn't add a new method, but instead adds a new option to nap(), "prepackaged".
When true, we optimistically assume we have all the right assets already, deserialize fingerprints.json, and run with it.

As it stands, this has two issues:

  • fingerprints.json is in the assets/ folder and as such is publicly accessible from the web.
  • nap() doesn't have a callback, yet the fingerprint loading happens some time after it returns.

Both can be fixed, one by picking a better spot to put the fingerprints (suggestions?), the other by using a .readFileSync call to grab the fingerprints (alternatively, by having nap() take a callback, but that seems more disruptive than necessary here.)

madhandennis commented

How about this:

  1. Have an option passed into nap indicating the target location of the fingerprint JSON file. So this way the user can decide where to store it and hence avoid the publicly accessible issue
  2. Pass in a JSON object to prepackaged instead of a boolean and that JSON object will be the fingerprint structure. So a user can just read in the file and pass it along thus avoid nap having to worry about reading the fingerprint JSON and providing a callback once ready.
craigspaeth commented

@henrit could you explain how it's possible for generated assets to differ without source assets changing? Or even better, provide an example of how to reproduce your issue with a version of nap before #40 (0.6.1 should work I think).

henrit commented

Sure. Let's have a source asset file named common.styl, that contains a few lines, the first of which is @import "reset"

Futher, let's have a nap setup like

nap({
  // ...
  mode: "production",
  assets: {
     css: {
        common: [ "common.styl" ]
     }
  }
}).package();

When processed, the import directive in common.styl will cause the stylus processor to look for a file named reset.styl and include it for processing.
While the file reset.styl is not explicitly known to nap itself, it is nonetheless used every time nap generates the asset, and it has the potential to change the generated common.css file even though common.styl remains untouched.

By running nap in production mode once on common.styl, then modifying reset.styl in a way that will change the output of common.css, then running nap again, you would get two different versions of common.css that have the exact same fingerprints in their filename, which confuses caching mechanisms in general, and CDNs in particular.

Does that make sense?

craigspaeth commented

Ahh, makes sense now. I think both use cases can be solved by simply running the preprocessors during fingerprinting, and reverting nap back to not having to call package to get fingerprints.

e.g.

fingerprintForPkg = (pkgType, pkgName) =>
  #...
  # Make sure the fingerprint is generated off processed assets and not source
  pkgContents = (contents for filename, contents of preprocessPkg(pkgName, pkgType)).join('')
  md5.update pkgContents
  #...

I'd prefer this to complicating the nap API by adding more options to work around this.

You can take a look at this implemented (among other house-cleaning things) on this branch: https://github.com/craigspaeth/nap/tree/fingerprint_back

Let me know if there might be a problem with this. Otherwise I'll merge this fix to master :)

craigspaeth commented

I've gone ahead and merged the fix I described above. Please re-open this if this doesn't solve your problems. Thanks for the feedback and help!

madhandennis commented

Hi Craig,

Sorry for the late reply. I have tried out the new version. So now my flow goes like this:

After development is over and I am ready to package my assets, I set nap mode to 'production' and call nap.package()

Now in my output directory I see all the compressed and fingerprinted packages.

So now when I run my app in production mode, I set nap mode to 'production' and start up the system, however all the files inside my asset directory are removed. Is this intentional? (rimraf.sync "#{process.cwd()}/#{@publicdir}/assets")

If I comment out the following lines, it works as per my expected flow.

Clear out assets directory

rimraf.sync "#{process.cwd()}/#{@publicdir}/assets"
fs.mkdirSync(process.cwd() + @_outputDir, '0755') unless @usingMiddleware

craigspaeth commented

Thanks for pointing this out. It is intentional to clear assets on initialization for development to be able to write to a cleared out directory. However this shouldn't be the case for production. I'm not sure if this is even necessary in general, but I was getting some failing tests if I removed it.

I've updated nap to clear the assets directory on init only for "development" mode and to clear out before calling package.

madhandennis commented

Awesome 👍 Thanks Craig :) Will try it out n let you know

madhandennis commented

Hi Craig,

Works perfectly for my setup now :) Just one more small fix. Looks like you still need rimraf for clearAssetsDir, however you have removed it from your package.json dependancies. So just need to add that back into the dependancy list and all works fine

craigspaeth commented

(-__-) derp. Thanks, accidentally removed it. Back in now, good catch.

madhandennis commented

Great! Thanks Craig! 👍