Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

make sure asset instances inherit their classes mimetype #56

Merged
merged 1 commit into from

3 participants

@vicapow

No description provided.

@noc7c9

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.

@vicapow

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.

@techpines
Owner

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?

@vicapow

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

@vicapow

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

@vicapow
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.

@techpines
Owner

@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.

@techpines
Owner

Yea it's an api issue.

Just got that post before :)

@noc7c9

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.

@noc7c9

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

@techpines
Owner

@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.

@noc7c9

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

@vicapow

@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)

@noc7c9

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.

@noc7c9

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?

@vicapow

should be good now :)

@techpines techpines merged commit 2580ab8 into techpines:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  lib/modules/angular-templates.coffee
@@ -4,7 +4,7 @@ uglify = require 'uglify-js'
Asset = require('../index').Asset
class exports.AngularTemplatesAsset extends Asset
- @mimetype: 'text/javascript'
+ mimetype: 'text/javascript'
create: (options) ->
options.dirname ?= options.directory # for backwards compatiblity
View
2  lib/modules/browserify.coffee
@@ -5,7 +5,7 @@ uglify = require('uglify-js')
Asset = require('../index').Asset
class exports.BrowserifyAsset extends Asset
- @mimetype: 'text/javascript'
+ mimetype: 'text/javascript'
create: (options) ->
@filename = options.filename
View
2  lib/modules/dynamic.coffee
@@ -12,7 +12,7 @@ class exports.DynamicAssets extends Asset
{@type, @urlPrefix, @options, @filter, @rewriteExt} = options
@urlPrefix ?= '/'
@urlPrefix += '/' unless @urlPrefix.slice(-1) is '/'
- @rewriteExt ?= mime.extensions[@type.mimetype] if @type.mimetype?
+ @rewriteExt ?= mime.extensions[@type::mimetype] if @type::mimetype?
@rewriteExt = '.' + @rewriteExt if @rewriteExt? and @rewriteExt[0] isnt '.'
@options ?= {}
@options.hash = @hash
View
2  lib/modules/jade.coffee
@@ -6,7 +6,7 @@ jade = require 'jade'
Asset = require('../index').Asset
class exports.JadeAsset extends Asset
- @mimetype: 'text/javascript'
+ mimetype: 'text/javascript'
create: (options) ->
@dirname = pathutil.resolve options.dirname
View
3  lib/modules/less.coffee
@@ -6,8 +6,7 @@ urlRegex = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/
urlRegexGlobal = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/g
class exports.LessAsset extends Asset
- @mimetype: 'text/css'
-
+ mimetype: 'text/css'
create: (options) ->
if options.filename
@filename = pathutil.resolve options.filename
View
2  lib/modules/sass.coffee
@@ -9,7 +9,7 @@ urlRegex = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/
urlRegexGlobal = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/g
class exports.SassAsset extends Asset
- @mimetype: 'text/css'
+ mimetype: 'text/css'
create: (options) ->
@filename = pathutil.resolve options.filename
View
2  lib/modules/snockets.coffee
@@ -3,7 +3,7 @@ Snockets = require 'snockets'
Asset = require('../index').Asset
class exports.SnocketsAsset extends Asset
- @mimetype: 'text/javascript'
+ mimetype: 'text/javascript'
create: (options) ->
@filename = pathutil.resolve options.filename
View
2  lib/modules/stylus.coffee
@@ -7,7 +7,7 @@ urlRegex = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/
urlRegexGlobal = /url\s*\(\s*(['"])((?:(?!\1).)+)\1\s*\)/g
class exports.StylusAsset extends Asset
- @mimetype: 'text/css'
+ mimetype: 'text/css'
create: (options) ->
@filename = pathutil.resolve options.filename
Something went wrong with that request. Please try again.