Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

make sure asset instances inherit their classes mimetype #56

Merged
merged 1 commit into from Mar 20, 2013

Conversation

Projects
None yet
3 participants
Contributor

vicapow commented Mar 20, 2013

No description provided.

Contributor

noc7c9 commented Mar 20, 2013

I actually used @mimetype to support DynamicAssets dynamic.coffee#L15.
Which I realize in hindsight was quite foolish of me since I could just lookup the prototype value.

Please change that line to @rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype?.
And thanks for catching this mistake.

Contributor

vicapow commented Mar 20, 2013

Ah! I understand now. You're code is correct. sails expects a mimetype variable to be on the asset instance.

see: https://github.com/balderdashy/sails/blob/master/lib/assets.js#L157

i was misunderstanding how coffeescript class/instance variables work.

class MyClass
    @myProperty

creates a property on the class, not the instance. (i, for whatever reason, thought that this would create a instance property with the '@' symbol in it and that's why the mimetype wasn't available to sails. what a silly mistake...)

so just making sure the instance gets its constructors mimetype should fix things.

Owner

techpines commented Mar 20, 2013

Asset-rack infers the mimetype from the url.

So an asset like this will automatically get a mimetype of text/css based on the url extension of .css:

new Asset
    url: '/some.css'
    contents: 'body {color: blue}'

So is this code change not necessary? Phantom bug?

Contributor

vicapow commented Mar 20, 2013

that works for .css and .js files but not with files that compile to a different extension like .less -> .css

Contributor

vicapow commented Mar 20, 2013

note that this issue only effects the sails code i've written to use LessAsset instead of CssAsset

Contributor

vicapow commented Mar 20, 2013

new LessAsset
    url : '/some-less-file.css'
    contents : 'body {color: blue}' # some less code

would currently serve a file at /some-less-file.css as compiled less code with the text/css mimetype but

new LessAsset
    url : '/some-less-file.less'
    contents : 'body {color: blue}' #some less code

would currently serve a file at some-less-file.less as compiled css but with a text/plain mimetype.

with my change, it would serve the same file with the same extension, but with text/css mimetype.

i guess you could argue that i'm using the api wrong by telling asset to serve the file with a less extension.

Owner

techpines commented Mar 20, 2013

@vicapow

If you go here in your sails branch:

https://github.com/vicapow/sails/blob/master/lib/assets.js#L83

I think the problem is that you are not specifying the correct url when you create the less asset.

url = '/' + pathutil
                .relative(sails.config.appPath, path).replace(/\\/g, '/');

That should actually be this:

url = '/' + pathutil
                .relative(sails.config.appPath, path)
                .replace(/\\/g, '/')
                .replace('.less', '.css');

Assets expect to know the exact url they are served from.

Owner

techpines commented Mar 20, 2013

Yea it's an api issue.

Just got that post before :)

Contributor

noc7c9 commented Mar 20, 2013

In my opinion the mimetype should be determined by the content not the url.

new LessAsset
  url : '/some-less-file.less'
  contents : 'body {color: blue}' #some less code

Here even with the '.less' extension the actual content is css; therefore the mimetype should be text/css not text/plain.

So I'm in favor of this pull request. Though it's your call @techpines.

Contributor

noc7c9 commented Mar 20, 2013

Of course this you still need to change to @rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype?.

Owner

techpines commented Mar 20, 2013

@noc7c9 I agree with your premise, but he shouldn't be specifying the url as .less. That leads to people serving their css code from the wrong url, but correct mimetype:

http://www.my-cool-site.com/some-less-file.less
content-type: text/css

I'm cool with merging this. But I think @vicapow should make the fix I mentioned above so that sails will serve from the correct urls.

Contributor

noc7c9 commented Mar 20, 2013

Yes I can't really think of any possible use case for serving with an incorrect url. =/
Still you never know ;)

Contributor

vicapow commented Mar 20, 2013

@techpines yep. thanks for pointing that point. i just pushed that change to my branch.

@rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype?

Maybe I'm getting lost in coffeescript again but I'm not sure why this is needed. and adding it causes the tests to fail.

This is because @type::mimetype -> references the variable mimetype from class's prototype (ie., this.type.prototype.mimetype in vallina js) which doesn't exist since it's only defined as a class variable (ie., this.type.mime in vallina js)

Contributor

noc7c9 commented Mar 20, 2013

Wait now I'm confused. Did you change your commit?

@rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype? is to go along with changing @mimetype: 'text/css' to mimetype: 'text/css' for all the assets.

The current commit is @mimetype ?= @.constructor.mimetype
This won't work as expected in some cases (or at least I think this isn't the expected behavior)
eg:

new LessAsset
  url: '/style.png'

A tad ridiculous I admit but this would be served with the image/png mimetype. Which isn't the desired behavior, I'm guessing.

Contributor

noc7c9 commented Mar 20, 2013

Let me see if I can make this a little less confusing.

  • First change @mimetype: 'text/css' to mimetype: 'text/css' for all assets

This would make these assets serve the correct mimetype regardless of the url.

  • However this would break DynamicAssets

So fix with @rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype?

This is what I believe is the behavior you were looking for, correct?

Contributor

vicapow commented Mar 20, 2013

should be good now :)

@techpines techpines added a commit that referenced this pull request Mar 20, 2013

@techpines techpines Merge pull request #56 from vicapow/master
make sure asset instances inherit their classes mimetype
2580ab8

@techpines techpines merged commit 2580ab8 into techpines:master Mar 20, 2013

@bryanchriswhite bryanchriswhite pushed a commit to shopbeam/asset-rack that referenced this pull request Nov 1, 2013

@techpines techpines Merge pull request #56 from vicapow/master
make sure asset instances inherit their classes mimetype
a2cdaf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment