ibm-js / requirejs-dplugins

AMD plugins for RequireJS
http://ibm-js.github.io/requirejs-dplugins/
Other
6 stars 9 forks source link

Svg plugin gets duplicate ids, fails loading icons #30

Open wkeese opened 9 years ago

wkeese commented 9 years ago

The svg! plugin may try to load two graphics with the same ID, and then it will effectively fail loading the second graphic. Technically, the second graphic will be loaded, but the problem is that it won't be accessible because of the ID collision. So, the plugin should avoid ID collisions.

Some of the cases where an ID collision happens are:

  1. Two svg graphics specify the same ID. Although one can avoid this in a single repository, it's impractical to avoid across multiple packages from different authors. For example, acme/combobox/arrow.svg and deliteful/dropdownbutton/arrow.svg might specify the same ID of arrow. If an app could load both the acme/combobox and deliteful/dropdownbutton widgets, it will hit the ID collision. In other words, it's bad for the same reason that it's bad to use global variables in libraries.
  2. An app is loads two versions of the same package. See the example on http://requirejs.org/docs/api.html#config-map with two versions of the package foo. Admittedly this is also a problem with CSS class names, but we shouldn't make the problem worse.

On a related note, after 5553e2001eb9a220cdc2b464bb563219096a76ad the svg! plugin requires that each svg graphic specify an ID. This should not be. The ID should be generated automatically. That's why this should be fixed before the 0.6 release: because it changes the API.

This problem has already been solved by AMD. The SVG plugin can get a unique identifier for a resource based on parentRequire.toUrl(), or something like that (see http://requirejs.org/docs/plugins.html#apiload). An alternate approach is to auto-generate unique id's for each graphic, for example icon1, icon2, icon3, icon4, ....

Unfortunately you are currently pre-generating sprite files on the server, which makes everything harder. It would be easier to follow the pattern in http://requirejs.org/docs/plugins.html#apiwrite, where you write each graphic's text to the layer file as (for example):

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

See https://github.com/wkeese/delite/blob/handlebars/handlebars.js#L326 for another example.

That approach will also solve the current inefficiency (also introduced in 5553e2001eb9a220cdc2b464bb563219096a76ad) where you unnecessarily download the requirejs-text/text plugin even when running builds.

wkeese commented 9 years ago

Since this alters the "API" it should be fixed before people start using the plugin.

clmath commented 9 years ago

Since we tell the user to use the id returned by the plugin, the api won't change if we decide to auto generates ids. So this can be fixed in a future release.

wkeese commented 9 years ago

On the contrary, the format of the .svg files (including specifying/not specifying an ID inside the .svg file) is part of the API.