Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Svg plugin gets duplicate ids, fails loading icons #30

Open
wkeese opened this issue Oct 23, 2015 · 3 comments
Open

Svg plugin gets duplicate ids, fails loading icons #30

wkeese opened this issue Oct 23, 2015 · 3 comments

Comments

@wkeese
Copy link
Member

wkeese commented Oct 23, 2015

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 5553e20 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 5553e20) where you unnecessarily download the requirejs-text/text plugin even when running builds.

@wkeese wkeese added this to the 0.6.0 milestone Oct 23, 2015
@wkeese
Copy link
Member Author

wkeese commented Oct 23, 2015

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

wkeese added a commit to wkeese/requirejs-dplugins that referenced this issue Oct 27, 2015
wkeese added a commit to wkeese/requirejs-dplugins that referenced this issue Oct 27, 2015
Assign unique ID to each SVG graphic loaded.
Fixes ibm-js#30.
clmath pushed a commit to clmath/requirejs-dplugins that referenced this issue Nov 4, 2015
@clmath
Copy link
Member

clmath commented Nov 5, 2015

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.

@clmath clmath removed this from the 0.6.0 milestone Nov 5, 2015
@wkeese
Copy link
Member Author

wkeese commented Nov 5, 2015

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

wkeese pushed a commit that referenced this issue Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants