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

Re-add prefix parameter on cleanupIds to fix unique ids #1763

Closed
wants to merge 1 commit into from

Conversation

yoriiis
Copy link

@yoriiis yoriiis commented Apr 8, 2023

Fixed: #1746

SVGO v3.0.0 removed the prefix parameter in the cleanupIds plugin in favor of prefixIds plugin. This created a regression with id conflicts with multiple SVGs.

The PR propose a revert of the bc07c48 commit to re-add the parameter and let user add a prefix with a custom hash.

Exemple implementation to resolve the id conflicts.

const crypto = require('crypto');

module.exports = {
    plugins: [
        {
            name: 'preset-default',
            params: {
                overrides: {
                    cleanupIDs: {
                        prefix: {
                            toString() {
                                return crypto.randomBytes(20).toString('hex').slice(0, 4);
                            }
                        }
                    }
                }
            }
        }
    ]
}

@yoriiis
Copy link
Author

yoriiis commented Apr 18, 2023

@TrySound and @GreLI could you take a look to this PR please? Thanks 🙏

@PurpleTape
Copy link

Any updates? 🥺🥺🥺

@yoriiis
Copy link
Author

yoriiis commented Oct 2, 2023

FYI @SethFalco

@SethFalco
Copy link
Member

SethFalco commented Oct 2, 2023

@yoriiis I'm working on improving our documentation. As soon as I'm done with that, I'll give this a look!

While reviewing this, I want to check out the prefixIds plugin and see if we can solve the issue there, rather than maintain the functionality twice or merge the plugins again, though.

One obvious bug I can see in Prefix IDs already, the plugin modifies IDs and references to IDs separately. It'll always break vectors when a function with random output is passed. I'd rather fix prefixIds than add this back to cleanupIds.

The prefixIds plugin should probably store a map of IDs that have been modified by the plugin, so if it encounters an ID again, it can take the previously generated one from the map instead.

Regarding docs, you can see progress here:
https://svgo.dev/docs/plugins/cleanupIds/

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

Successfully merging this pull request may close these issues.

Unique id is broken since v3
4 participants