Create unique ID prefix within SVGs to prevent ID collisions by SilverFox70 · Pull Request #108 · jhamlet/svg-react-loader

SilverFox70 · 2018-10-17T19:13:31Z

Haven't had time to go back over this with a fine tooth comb since I made these changes, but it is working fine on two separate production sites at this point. It could be made more robust by implementing a 'usable' default prefix based on something like shortid, but that would meaning adding dependencies: see lib/sanitize/filters/unique-svg-ids.js at line line 7 for more info.

Added fix and test cases for paths names including consecutive dashes…

Feature/create unique id prefix

chore: remove logging line

feat: add another test and update variable name in test file

Feat/add test

woutervanvliet

So good to see a fix for this to be pending review ;).

I'm not an SVG expert, but your filter looks fine to me. PR would need to be cleaned for all "fork" references though.

Note: I'm neither contributor nor maintainer for this project, just passing by and hoping to get this PR merged asap as I need it myself.

woutervanvliet · 2018-10-19T08:19:11Z

README.md

-are acted on by the loader version of `svg-react`.
+are acted on by the loader version of `svg-react`. 
+
+This fork has an added filter which creates 'unique' IDs and mask, fill, and xlink:href


You probably don't want your "this fork" mentions merged into the main package.

woutervanvliet · 2018-10-19T08:20:50Z

package.json

  "repository": {
    "type": "git",
-    "url": "git+https://github.com/jhamlet/svg-react-loader.git"
+    "url": "git+https://github.com/SilverFox70/svg-react-loader"


Also probably not a change that should be merged up ;)

woutervanvliet · 2018-10-19T08:22:35Z

test/unit/sanitize/filters/unique-id-prefix.js

+/*globals describe, it*/
+require('should');
+
+describe('svg-react-loader/lib/sanitize/filters/unique-svg-ids', () => {


Maybe you can also include a simple test in test/integration/test.js on an actual svg file.

woutervanvliet · 2018-10-19T08:24:42Z

lib/loader.js

    var raw            = params.raw;
    var xmlnsTest      = params.xmlnsTest;
    var classIdPrefix  = params.classIdPrefix || false;
+    var uniqueIdPrefix = params.uniqueIdPrefix || false;


Would it make sense to use the value, after interpolation, of classIdPrefix as a default?

(interpolation will be fixed as soon as #107 is merged, btw)

I'm not sure if that would work as intended.

Using the same prefix would across all instances and files would collide too, no?

It might be more relevant to allow tokenizing within a default string, a la: [name]-[hash]-

jhamlet · 2018-10-19T14:14:25Z

README.md

-are acted on by the loader version of `svg-react`.
+are acted on by the loader version of `svg-react`. 
+
+This fork has an added filter which creates 'unique' IDs and mask, fill, and xlink:href


jhamlet · 2018-10-19T14:15:10Z

package.json

  "repository": {
    "type": "git",
-    "url": "git+https://github.com/jhamlet/svg-react-loader.git"
+    "url": "git+https://github.com/SilverFox70/svg-react-loader"


jhamlet · 2018-10-19T14:15:23Z

test/unit/sanitize/filters/unique-id-prefix.js

+/*globals describe, it*/
+require('should');
+
+describe('svg-react-loader/lib/sanitize/filters/unique-svg-ids', () => {


jhamlet · 2018-10-19T14:24:40Z

lib/sanitize/filters/unique-svg-ids.js

+            this.update(value);
+        }
+
+        // Find all IDs and update with filename prefix


Couple of thoughts:

Are these the only attributes that need to be sanitized for the id prefix? I can think of (at least) a couple more: clip-path, use, ?...

There is a bit of repetition going on here for some of the attributes... you might be able to break this down to an array of objects with a test property, and then a operator function property which takes the node reference and other parameters it may need (prefixId, etc...).

By abstracting the update away into separate functions, and operating over a list of definitions, it becomes trivial to handle new cases (just add them to the list).

Bonus Points: Allow for end users to configure their loader with additional tests (exporting the utility operator functions too).

Thanks for looking this over and providing the feedback. I will endeavor to get back around to this and resolve as soon as I am able.

Totally agree about abstracting items into separate functions. As the old saying goes, "I would have written a shorter letter if I'd had more time". I was able to carve out a few minutes to add a couple of tests and continue the not-abstract, brute force methods for some expanded cases.

Perhaps I will have more time in the future to revisit, refactor, and make sure that all cases of ID reference are handled

Hi!
if i try insert svg with some custom filter

<svg xmlns="http://www.w3.org/2000/svg" width="16" height="22" viewBox="0 0 16 22"> <defs> <filter id="a" width="200%" height="200%" x="-50%" y="-50%" filterUnits="objectBoundingBox"> .... </filter> </defs> <g fill="none" fill-rule="evenodd" filter="url(#a)" transform="matrix(-1 0 0 1 19 -3)"> ... </g> </svg>

loader convert only <filter id="test__a", but reference to filter - original url <g filter="url(#a)"

@SilverFox70 created pull request to resolve this issue SilverFox70#5

…ntegration test for above

jhamlet · 2018-10-26T14:22:16Z

lib/options.js

            }));
    }

+    if (options.uniqueIdPrefix) {


This option sounds and feels like it should default to true

any reasons for not making it default to being on?

Yes, I would agree with that. Super busy today, but if I can carve out ten minutes I can update that.

jhamlet · 2018-10-26T14:24:52Z

As far as I can see this is looking good.

I made a couple more comments about defaulting to having this feature on, instead of off.

I'll merge it some time over the weekend.

Thanks,

;-j

jedrichards · 2019-02-04T18:50:42Z

Is there anything I can do to help get this feature merged?

SilverFox70 · 2019-02-04T19:05:00Z

I just added the last change - to make uniqueIdPrefix default to true.

cupojoe and others added 14 commits March 20, 2018 11:23

Merge pull request #1 from cupojoe/fix/error-loading-svg-w-two-dashes

fac069f

Added fix and test cases for paths names including consecutive dashes…

feat: create filter module to prefix svg filename to IDs within SVGs

c6e37ef

update [readme] clarify what uniqueIdPrefix actually does

a4490b9

chore: clean up logs and dead code

43c574a

Merge pull request #1 from SilverFox70/feature/creeateUniqueIdPrefix

3bc85ed

Feature/create unique id prefix

chore: remove logging line

72ab380

Merge pull request #2 from SilverFox70/feature/creeateUniqueIdPrefix

87b6ed3

chore: remove logging line

feat: add simple tests

1cf6149

chore: minor clean up

e07b61d

feat: add another test and update variable name in test file

2b74ac0

Merge pull request #3 from SilverFox70/feat/add-test

088bb2c

feat: add another test and update variable name in test file

docs: update readme

6e59f5e

docs: update readme to fix typo

61e55b6

Merge pull request #4 from SilverFox70/feat/add-test

a065720

Feat/add test

woutervanvliet suggested changes Oct 19, 2018

View reviewed changes

jhamlet suggested changes Oct 19, 2018

View reviewed changes

SilverFox70 added 3 commits October 19, 2018 10:42

docs: update readme and package.jsonfor merge

d193241

update: include simple integration test to assert unique id prefix works

b863896

feat: handle clipPath, use, and other case of href with SVGs; added i…

8c1fc94

…ntegration test for above

jhamlet reviewed Oct 26, 2018

View reviewed changes

jhamlet approved these changes Oct 26, 2018

View reviewed changes

feat: make uniqueIdPrefix default to true

f7e91c6

Strangehill mentioned this pull request Jul 24, 2020

query parameter classIdPrefix: true not working jacobmischka/gatsby-plugin-react-svg#40

Open

Comments

Conversation

SilverFox70 commented Oct 17, 2018

Uh oh!

woutervanvliet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anagami Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhamlet commented Oct 26, 2018

Uh oh!

jedrichards commented Feb 4, 2019

Uh oh!

SilverFox70 commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anagami Mar 25, 2019 •

edited

Loading