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

Active link object mutation #270

Open
estevanmaito opened this issue Aug 4, 2022 · 0 comments
Open

Active link object mutation #270

estevanmaito opened this issue Aug 4, 2022 · 0 comments

Comments

@estevanmaito
Copy link

Problem

The following snippet will underline both links if I'm at #/settings and none if I'm at #/settings/settings:

<script>
    const activeLinkStyle = { className: "underline" };
</script>

<a href="#/settings" use:active={activeLinkStyle}>Docsets</a>
<a href="#/settings/settings" use:active={activeLinkStyle}>Settings</a>

Two ways to circumvent this bug are the following:

<script>
    // 1. duplicate the object for each link
    const activeLinkStyle1 = { className: "underline" };
    const activeLinkStyle2 = { className: "underline" };
</script>

<!-- 2. duplicate the attributes in the link -->
<a href="#/settings" use:active={{ className: "underline" }}>Docsets</a>
<a href="#/settings/settings" use:active={{ className: "underline" }}>Settings</a>

This is happening because the object passed to the active function is being directly mutated, so if you share the same object with multiple links, as is the case of my first example where I just want every link to have the same style, every link will get the same path. This is the location of the code: https://github.com/ItalyPaleAle/svelte-spa-router/blob/916347dbef1eb429332bca8375dae549169e665a/active.js#L54:L65

Solution

I'd love to create a PR to fix it, but I couldn't set up the environment, so if someone is interested in fixing it, the solution would be as the following:

Instead of this (current code):

export default function active(node, opts) {
    // Check options
    if (opts && (typeof opts == 'string' || (typeof opts == 'object' && opts instanceof RegExp))) {
        // Interpret strings and regular expressions as opts.path
        opts = {
            path: opts // <-- ⛔️ this is mutating the object, which is a problem if it's shared
        }
    }
    else {
        // Ensure opts is a dictionary
        opts = opts || {}
    }

Do this:

export default function active(node, opt) { // <-- rename the argument to make less changes
    const opts = {...opt} // <-- clone the object
    // Check options
    if (opts && (typeof opts == 'string' || (typeof opts == 'object' && opts instanceof RegExp))) {
        // Interpret strings and regular expressions as opts.path
        opts = {
            path: opts
        }
    } // <-- no need for else anymore
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

1 participant