cujojs / curl

curl.js is small, fast, extensible module loader that handles AMD, CommonJS Modules/1.1, CSS, HTML/text, and legacy scripts.
https://github.com/cujojs/curl/wiki
Other
1.89k stars 216 forks source link

CSS loader plugin breaks on URLs containing commas #211

Open jcreenaune opened 10 years ago

jcreenaune commented 10 years ago

The CSS loader plugin splits given resources on ",", so attempting to load eg:

css!a,b,c

will attempt to load 3 resources, a.css, b.css and c.css.

The CSS loader plugin should be able to download resources containing a comma in the URL - it's a valid character in URLs.

I'm not sure why this behaviour exists. The JS loader plugin doesn't have the same behaviour - haven't checked other plugins. However, it probably doesn't make sense to change the behaviour now ... would you be happy with a PR that makes splitting on comma optional (defaulting the current behaviour if nothing is specified?)

scothis commented 10 years ago

As a work around, you can try encoding the comma. So css!a,b,c becomes css!a%2Cb%2Cc which in turn loads a,b,c.css.

unscriptable commented 10 years ago

Hey @jcreenaune,

The comma splitting is intentional to allow precise ordering of stylesheets (which is very important in css, as you know). If you don't like @scothis's work-around, there might be another option if you don't plan to concat the css into a bundle. If this is the case, you could use the link! plugin. It's strictly a run-time plugin and works similarly, but without the comma-delimited stylesheets. If you want to keep using the css! prefix, you can map a path: paths: { css: 'curl/plugin/link' }.

Some sort of config option (as you suggested) is more work than you think and is problematic. Here's why:

The AMD specification defines the plugin's normalize method as taking two parameters: resourceId (everything after the "css!") and normalize (the loaders function for normalizing AMD modules). The configuration object is not passed to this function as it is with the load() function.

There is, however, a way to obtain the plugin's config from within the plugin module. curl.js (and requirejs) support the optional module.config() function to gain access to a module's configuration.

define(['module'], function (module) {
    var config = module.config();
    // do something with the config
});

This isn't hard to add to the css plugin module, but now we've got to figure out how to determine when to use module.config() or the config that's passed as the last param to the plugin's load() method. These two config objects are different:

Imagine the following scenario:

curl.config({
    plugins: {
        css: {
            cssDelimiter: '|' // split on vertical bar
        }
    },
    packages: {
        app: { 
            location: 'app/',
            config: {
                cssDelimiter: '' // falsey == don't split?
            }
        }
    }
});

When loading "css!app/foo/styles.css,other.css", the module.config() object will specify "|" and the config object passed to load() will specify "" (blank).

Most people don't have sophisticated configurations like this, but some do. Seems a bit newbish to implement something with pitfalls like this.

Maybe we could use a suffix on the id? e.g. "css!app/foo/styles.css,other.css!nosplit" I've been trying to get away from these, but this would work. The "!nosplit" will be sent to both normalize() and load() in the first parameter.

Got any other ideas?

Regards,

-- John

unscriptable commented 10 years ago

We could also do something non-standard, but sensible: pass a config object to normalize().

unscriptable commented 10 years ago

This issue is related to #234.