Svg: fixing #29 and #30

[tkrugg]

No description provided.

[wkeese]

I looked over this PR and added a number of line-item comments. The PR has a number of improvements, but it's not addressing #30. Your proposal is that every SVG attempt to specify a unique id. That doesn't work for a number of reasons:

• Different authors in different projects will create SVG files, so it's impossible to coordinate every SVG file across every project to have a unique ID. Yet, all those SVG files may be loaded by the same application, causing an ID conflict.
• Even if every SVG file had a unique ID, if an application loaded two versions of a library (for example, deliteful 0.8 and deliteful 0.9), where each version was built into a layer, you will end up with ID conflicts.
• It's inconvenient for SVG authors to manually specify IDs. AMD javascript files don't specify IDs so SVG files shouldn't have to either.

This is basically what I outlined already in #30, and IMO illustrates why it would be simpler to inline each icon's text into the javascript layer, and then assign an ID when load() is called according to the normalized moduleName.

[tkrugg]

It's inconvenient for SVG authors to manually specify IDs. AMD javascript files don't specify IDs so SVG files shouldn't have to either.

I completely agree there is an issue.
Following my latest fix, the API now returns the correct id, so when we will fix the remaining of the issue, we will be able to do so without breaking the current API

[wkeese]

I completely agree there is an issue.
Following my latest fix, the API now returns the correct id, so when we will fix the remaining of the issue, we will be able to do so without breaking the current API

It would actually be better in the meantime if you continued to default the ID to the filename, or perhaps path.replace(/\//g, "-"), so that users don't have to add temporary IDs to their SVG files. Also, you should stop claiming this PR fixes #30. Explicitly specifying IDs does not solve the majority of issues I listed in #30.

As for a real fix to #30, there's still the very simple approach I recommended to leverage the text! plugin, see https://github.com/ibm-js/requirejs-dplugins/blob/9889f3a429cf7d0349c71568d258daf9dc233f2b/svg.js. I know the downside is that it inlines the boilerplate text (DOCTYPE etc) for each graphic, but at least it's moving in the right direction.

A more efficient (although harder) approach would be to have a custom write() method that only added the necessary text to the layer. I'm not an expert on builds, but I notice the text! plugin has this code:

   write: function (pluginName, moduleName, write, config) {
        if (buildMap.hasOwnProperty(moduleName)) {
            var content = text.jsEscape(buildMap[moduleName]);
            write.asModule(pluginName + "!" + moduleName,
                           "define(function () { return '" +
                               content +
                           "';});\n");
        }
    },

IIUC it defines a module in the layer called (for example) "requirejs-text/text!foo/bar.svg", where that module's content is simply:

define(function(){
   return "...<svg>....</svg>";
});

Presumably you could have your own write() method that added a module like "requirejs-dplugins/svg!foo/bar.svg" to the layer, with content like:

define(["requirejs-dplugins/svg"], function(svg){
   return svg.addGraphic("0 0 32 32", "<polygon.../><path.../>");
});

Obviously it requires a utility addGraphic(viewbox, svgText) method that creates a <symbol> based on the specified viewBox and svgText, assigns a unique ID, adds it to the DOM, and then returns the ID.

[clmath]

Merged in 5553e20