assetgraph / assetgraph-builder

AssetGraph-based build system for web apps and web pages.
BSD 3-Clause "New" or "Revised" License
489 stars 42 forks source link

relations pointing to non-existing assets should not be added to the manifest file and shouldn't break the build process #42

Closed mreinstein closed 11 years ago

mreinstein commented 11 years ago

I have a special case in my index.html file; there is one javascript that is specified as:

<script src="user"></script>

this points at a purposely non-existent static file, which forces it to be loaded from the API. so there is no file named 'user' in my assets directory. This causes buildProduction to break.

What I need is a way to provide some hinting and tell assetgraph builder "leave this one alone in the index.html file. just ignore it."

any ideas?

papandreou commented 11 years ago

That's a bug. Relations pointing at non-existent assets should be left alone. I think what's happening is that AssetGraph cannot work out the asset type of user because it doesn't have an extension, so it tries to load it in order to detect the mime type using file -b --mime-type. I'll take a look at it!

Temporary workaround:

<script>document.write('<script src="user"><\/script>');</script>

Referring to user.js instead would probably also work.

mreinstein commented 11 years ago

@papandreou cool that workaround fixed it. Thanks!

mreinstein commented 11 years ago

Question, is it also a bug that non-existent assets are getting put into the generated manifest? In my example, user.js is not found, but when I add the --manifest command line option I see user.js get into the manifest file. This breaks my app because its telling the browser to cache a resources that needs to be dynamic, which is why its intentionally a missing reference in the first place.

papandreou commented 11 years ago

Yeah, that's a bug, fixed in https://github.com/One-com/assetgraph/commit/5402b9e0cb3da6a497fd8a7be6424cad2c4e54bb -- thanks :)

mreinstein commented 11 years ago

I'm thinking it makes sense to rename this issue to "relations pointing to non-existing assets should be ignored" and tag it as a bug. I want to make sure these tickets are actionable and closable.

papandreou commented 11 years ago

Sure, go for it!

Munter commented 11 years ago

I just want to make sure that this issue won't remove the functionality that tells me when I am actually pointing to a missing file during a static build. I find it quite useful that I can't build an application that isn't complete.

mreinstein commented 11 years ago

@munter absolutely, I don't mean to imply that failed links should fail silently. What Ireally mean is that if an asset can't be found, it shouldnt be putting non existent references into the manifest file (and any other places.)

So in the case of my app, the references to backbone.js, underscore.js and user.js wouldnt appear in the manifest. I'll update the title to try to make this more clear.

mreinstein commented 11 years ago

@papandreou I think this may have regressed in 1.2.2. I updated to 1.2.2, reran my deploy script, and I'm seeing the error that was showing up before you introduced patch https://github.com/One-com/assetgraph/commit/5402b9e0cb3da6a497fd8a7be6424cad2c4e54bb

Here's the error: file:///root/whim/web/public/user.js: Error: ENOENT, open '/root/whim/web/public/user.js

mreinstein commented 11 years ago

I think I found the problem. did someone publish an old version to npm and mark it as the newest? when I run npm install assetgraph-builder it attempts to install version 1.2.2. but I have 1.4.0 installed on another server. I think this needs to be fixed in the npm registry.

Munter commented 11 years ago

This is how npm works. Never depend on "latest", since that means the chronologically latest upload to npm, no matter the version. It's pretty standard practice to have legacy branches with back ported patches, so you are going to run into this in lots of places.

The solution is to depend on ~1.4.x, though I recommend being very specific about the version at the moment, since each minor version also contains improvements. I recommend "=1.4.0" and keep updating it when you see interesting stuff coming in.

papandreou commented 11 years ago

Ah, right. I have some projects still running on assetgraph-builder 1.2.x and had to backport a fix, so I published 1.2.2 as a "maintenance release" from the 1.2 branch a couple of days ago.

For some odd reason, npm install <packageName> means npm install <packageName>@latest, which again means "install the version that was published most recently" not "install the highest version number".

I've read up on it a little and added "tag": <branchName> to the legacy branches. According to https://npmjs.org/doc/config.html that means it won't update the latest tag when new versions are published from those branches. I've also used the npm tag command to update the latest tag of assetgraph-builder to point at 1.4.0.

Munter commented 11 years ago

I didn't know about that configuration. It seems many other authors don't either. Thanks for notifying.

papandreou commented 11 years ago

Closing. I believe the original issue has been fixed -- along with the others that were raised during the discussion.