DEV Community

Tan Li Hau
Tan Li Hau

Posted on • Originally published at lihautan.com on

Contributing to Svelte - Fixing issue #4392

#svelte #opensource #javascript

Background

As Svelte gains more attention, I find that more and more people are interested in contributing to Svelte.

Of course, contributing to Svelte, does not mean to contribute only in code, it could be:

  • answering questions about Svelte, on social media, Stack Overflow, or Discord
  • improving Svelte docs, or write tutorials and articles about Svelte
  • organising and speaking in meetups about Svelte

For those who want to contribute in code, most people are unsure where to start. So I wrote The Svelte Compiler Handbook as an overview of the Svelte source code.

However, today, I want to try a different tone.

I am going to tell you an anecdote on how I investigated and fixed a bug in Svelte.

I documented down my train of thoughts as detailed as possible.

I hope this gives anyone who is reading, a glimpse on how to work on the Svelte source code.

The story begins

I was combing through bugs on GitHub, and found this rather interesting bug:

Select multiple value does not get set with spread props #4392

Adding any type of spread, even an empty object {...{}}, causes the value not to be set:

<script>
  let value = ['Hello', 'World'];
</script>

<select multiple {value} {...{}}>
  <option>Hello</option>
  <option>World</option>
</select>
Enter fullscreen mode Exit fullscreen mode

To reproduce: REPL.

Verifying the bug

I clicked into the REPL and tried to understand about the bug.

I found that if the <select multiple> has spread attribute {...any}, the value attribute will not be reactive. Changes in the value of value will not be reflected to the <select>.

I noticed the REPL link uses the version 3.18.1, it’s not the latest version of Svelte. At the point of writing, Svelte is at 3.22.3. I tried removing the ?version=3.18.1 from the query params to verify whether the bug has fixed, and realised that the bug is still there. (Great! Something interesting to investigate into.)

To understand the current status of the issue, I read through the comments. According to Conduitry, the issue is related to Radio/checkbox input with bind:group and spread props makes variable undefined #3680 and can be fixed together. However, the issue #3680 was fixed and closed, yet this issue is still open.

Nevertheless, I read through the PR for the closed issue #3680, roughly understand how it was fixed and hopefully it can give me some inspirations on this issue.

Investigating the bug

Once I verified that the behavior described in the issue is unexpected and reproducible in the latest version of Svelte, I copied the REPL code into my local machine to investigate.

I have a test-svelte folder ready in my local machine, where I created using Svelte Template. I have npm linked my local Svelte clone to the test-svelte folder, so I can rebuild test-svelte anytime with the latest changes done to my Svelte clone.

- /Projects
  - svelte <-- cloned from https://github.com/sveltejs/svelte
  - test-svelte <-- initialised with Svelte Template
    - node_modules/svelte <-- symlink to `/Projects/svelte`
Enter fullscreen mode Exit fullscreen mode

I have yarn dev running in the Svelte folder, so any changes I make gets compiled immediately.

I prefer to build test-svelte and serve it with http-server rather than start a dev server test-svelte in watch mode. That allows me to

  • Run the http-server in the background while tweaking the Svelte code or the test-svelte app.
  • Not having to restart the dev server whenever I’ve made changes to the Svelte code
  • Able to inspect and modify bundle.js without worrying that accidentaly save in the test-svelte app will overwrite the bundle.js

Looking at the different bundle.js generated from with {...spread} attributes and without spread attributes

<script>
  let value = ['Hello', 'World'];
</script>

<!-- with spread -->
<select multiple {value} {...{}}>
  <option>Hello</option>
  <option>World</option>
</select>

<!-- without spread -->
<select multiple {value}>
  <option>Hello</option>
  <option>World</option>
</select>
Enter fullscreen mode Exit fullscreen mode

I found the following diffs in the bundled output:

+ let select_levels = [{ multiple: true }, { value: /*value*/ ctx[0] }, {}];
+   let select_data = {};
+   for (let i = 0; i < select_levels.length; i += 1) {
+ select_data = assign(select_data, select_levels[i]);
+   }
- let select_value_value;

  return {
    c() {
      /* ... */
      set_attributes(select, select_data);
    },
    m(target, anchor) {
      /* ... */
- select_value_value = /*value*/ ctx[0];

- for (var i = 0; i < select.options.length; i += 1) {
- var option = select.options[i];
- option.selected = ~select_value_value.indexOf(option.__value);
- }
    },
- p: noop,
+ p(ctx, [dirty]) {
+ set_attributes(select, get_spread_update(select_levels, [{ multiple: true }, dirty & /*value*/ 1 && { value: /*value*/ ctx[0] }, {}]));
+ },
    /* ... */
  };
Enter fullscreen mode Exit fullscreen mode

Well, I know I haven’t cover how spread attribute works in any of my “Compile Svelte in your Head” articles, but the general idea is that, Svelte builds an array of attributes, and then apply it to the element / Component.

For example, if we write the following in Svelte

<div foo="foo" {...bar} baz="baz" {...qux} />
Enter fullscreen mode Exit fullscreen mode

It gets compiled to something like this:

const levels = [{ foo: 'foo' }, bar, { baz: 'baz' }, qux];
// build the attribute maps
const data = {};
for (let i = 0; i < levels.length; i++) {
  data = Object.assign(data, levels[i]);
}

// set attribute to element
for (const attributeName in data) {
  div.setAttribute(attributeName, data[attributeName]);
}

// if `bar` changed
const updates = get_spread_update(levels, [false, bar, false, false]);
// updates will return the updates needed to make, in this case, the diff in `bar`, eg: { aa: '1' }
for (const attributeName in updates) {
  div.setAttribute(attributeName, updates[attributeName]);
}
Enter fullscreen mode Exit fullscreen mode

So, this roughly explains the additional code added into the bundle.js for handling spread attributes.

However the code that is removed, is something I am not familiar with.

// in `mount` method
for (var i = 0; i < select.options.length; i += 1) {
  var option = select.options[i];
  option.selected = ~select_value_value.indexOf(option.__value);
}
Enter fullscreen mode Exit fullscreen mode

It seems like we are trying to set option.selected after we mount the <select> element. Not sure how important is that to us.

To verify that the bug is because that the above code snippet is missing when having a spread attribute, I tried adding the code snippet into the bundle.js manually, and refresh the page.

// ...
  m(target, anchor) {
    insert(target, select, anchor);
    append(select, option0);
    append(select, option1);
    for (var i = 0; i < select.options.length; i += 1) {
      var option = select.options[i];
      option.selected = ~ctx[0].indexOf(option.__value);
    }
  },
// ...
Enter fullscreen mode Exit fullscreen mode

Instead of ~select_value_value.indexOf(...), I changed it to ~ctx[0].indexOf(...), as select_value_value wasn’t created when using spread attribute.

…and it works!

Fixed

So, now we know that the bug is caused by missing setting option.selected on mount, now its time to figure out what the code snippet is not generated when there’s a spread attribute.

To quickly find out why something is not generated, I tried to look for where it is generated, figuring out probably whether certain condition was not set correctly to cause the Svelte compiler to omit out the code snippet.

To find the right place to start looking is an art. Usually I try to global search a small snippet of code that is most likely static , something that has no variable name, for example:

  • .indexOf(option.__value)
  • .options.length;
  • .selected = ~

The only search result I got when searching for .indexOf(option.__value) is in src/runtime/internal/dom.ts

export function select_options(select, value) {
  for (let i = 0; i < select.options.length; i += 1) {
    const option = select.options[i];
    option.selected = ~value.indexOf(option.__value);
  }
}
Enter fullscreen mode Exit fullscreen mode

Anything within src/runtime/ are helper functions that are referenced from the output code, to reduce the output code size. Hmm… probably we should reuse the select_options helper function:

// ...
  m(target, anchor) {
    insert(target, select, anchor);
    append(select, option0);
    append(select, option1);
-   for (var i = 0; i < select.options.length; i += 1) {
-     var option = select.options[i];
-     option.selected = ~ctx[0].indexOf(option.__value);
-   }
+   select_options(select, select_value_value);
  },
// ...
Enter fullscreen mode Exit fullscreen mode

Anyway, src/runtime/internal/dom.ts is not where I am looking for, so I tried searching .options.length

src/compiler/compile/render_dom/wrappers/Element/Attribute.ts

updater = b`
  for (var ${i} = 0; ${i} < ${element.var}.options.length; ${i} += 1) {
    var ${option} = ${element.var}.options[${i}];

    ${if_statement}
  }
`;
block.chunks.mount.push(b`
  ${last} = ${value};
  ${updater}
`);
Enter fullscreen mode Exit fullscreen mode

Yes, this is most likely where it is.

Firstly, let me update the updater to use the src/runtime/ select_options helper instead:

if (is_multiple_select) { updater = b`@select_options(${element.var}, ${last});`;} else { updater = b`@select_option(${element.var}, ${last});`;}
block.chunks.mount.push(b`
  ${last} = ${value};
  ${updater}
`);
Enter fullscreen mode Exit fullscreen mode

The b`...`, is called a tagged template, where the b is a function that takes in the template literal and return something. In this case, the b function returns an Abstract Syntaxt Tree (AST).

The b function comes from code-red, a utility to generate a JavaScript AST node. Beside b, code-red provides a few helper functions:

  • b returns a block node
  • x returns an expression node
  • p returns a object property node

These helper functions are useful in generating code in Svelte compiler, particularly because the placeholder itself can takes in another AST node:

const node = { type: 'Identifier', name: 'foo' };
const code = b`const ${node} = 1;`; // returns an AST node for `const foo = 1;`
Enter fullscreen mode Exit fullscreen mode

@ in front of @select_option is a convention in Svelte, where it will get replaced to refer to helpr functions in src/runtime/ before writing the generated AST out:

const code = b`@foo(bar)`;
// turns into

import { foo } from 'svelte/internal';
foo(bar);
Enter fullscreen mode Exit fullscreen mode

Coming back to figure out why this piece of code is not executed when there’s a spread attribute,

if (is_legacy_input_type) { // ...
} else if (is_select_value_attribute) { if (is_multiple_select) {
    updater = b`@select_options(${element.var}, ${last});`;
  } else {
    updater = b`@select_option(${element.var}, ${last});`;
  }

  block.chunks.mount.push(b`
    ${last} = ${value};
    ${updater}
  `);
} else if (is_src) {
  // ...
}
Enter fullscreen mode Exit fullscreen mode

I tried adding console.log before the if statement, to figure out the value for is_legacy_input_type and is_select_value_attribute:

console.log( 'is_legacy_input_type:', is_legacy_input_type, 'is_select_value_attribute:', is_select_value_attribute);if (is_legacy_input_type) {
  // ...
}
Enter fullscreen mode Exit fullscreen mode

To my surpise, there was no log. AttributeWrapper#render wasn’t executed.

I tried removing the spread attribute, and verified from the log that the AttributeWrapper#render method was indeed executed when there’s no spread attribute.

To figure out the caller of the AttributeWrapper#render method, I added console.trace at the top of the method:

render(block: Block) {
  console.trace('trace'); // ...
}
Enter fullscreen mode Exit fullscreen mode 
Trace: trace
  at AttributeWrapper.render (/Projects/svelte/compiler.js:8269:11) at /Projects/svelte/compiler.js:10749:14
  at Array.forEach (<anonymous>)
  at ElementWrapper.add_attributes (/Projects/svelte/compiler.js:10748:19) at ElementWrapper.render (/Projects/svelte/compiler.js:10472:8)
  at /Projects/svelte/compiler.js:10454:11
  at Array.forEach (<anonymous>)
  at ElementWrapper.render (/Projects/svelte/compiler.js:10453:24)
  at FragmentWrapper.render (/Projects/svelte/compiler.js:13030:18)
  at new Renderer (/Projects/svelte/compiler.js:13112:17)
Enter fullscreen mode Exit fullscreen mode

This brought me to src/compiler/compile/render_dom/wrappers/Element/index.ts

add_attributes(block: Block) {
  // Get all the class dependencies first
  this.attributes.forEach((attribute) => {
    if (attribute.node.name === 'class') {
      const dependencies = attribute.node.get_dependencies();
      this.class_dependencies.push(...dependencies);
    }
  });

  if (this.node.attributes.some(attr => attr.is_spread)) {
    this.add_spread_attributes(block);
    return;
  }

  this.attributes.forEach((attribute) => {
    attribute.render(block); });
}
Enter fullscreen mode Exit fullscreen mode

If there’s a spread attribute, it will call the this.node.attributes.some(attr => attr.is_spread) method instead of calling attribute.render(block), so that’s probably why AttributeWrapper#render wasn’t called.

I looked into the method add_spread_attributes, found out it contain only the code about handling spread attributes as I explained earlier. It didn’t have any code related to select_options, so I figured that, maybe <select multiple> with spread attribute is an edge case that wasn’t handled currently at all.

So, I tried to add a special check for this case at the bottom of the add_spread_attributes method:

add_spread_attributes(block: Block) {
  // ...
  // for `<select>` element only
  if (this.node.name === 'select') {
    block.chunks.mount.push(
      b`@select_options(${this.var}, ${data}.value);`
    );
  }
}
Enter fullscreen mode Exit fullscreen mode

As mentioned in the The Svelte Compiler Handbook, a block is where it keeps the code to generate the create_fragment function. The return object of the create_fragment function contains various method as mentioned in Compile Svelte in your Head, such as c(), m() and d(). To add code into different method, you can push them into the array in block.chunks, for example:

// push `const foo = 1` to `m()`
block.chunks.mount.push(b`const foo = 1`);

// push `const bar = 2` to `c()`
block.chunks.create.push(b`const bar = 2`);
Enter fullscreen mode Exit fullscreen mode

I tried adding @select_options(...) into the m() method and yup, the <select> element is pre-selected correctly!

Fixing the bug

To ensure the bug is fixed, I need to come up with a test.

Usually I come up with test cases that try to entail various scenario I can imagine.

In this example, we’ve manually tested the case where the <select multiple {value} {...{}}>, the value is set correctly during initialisation. but have we check the case where:

  • we update the value of value, will the <select> get updated accordingly?
  • if the value is overriden by the spreaded attribute, eg <select mutliple {value} { ...{value: []} }>?

Ideally, the test cases come up should be failed before the fix, and passed after the fix.

So here’s the test case I came up:

<script>
  let value = ['Hello', 'World'];
  export let spread = {};
</script>

<select multiple {value} {...spread}>
  <option>Hello</option>
  <option>World</option>
</select>

<input type="checkbox" value="Hello" bind:group={value}>
<input type="checkbox" value="World" bind:group={value}>
Enter fullscreen mode Exit fullscreen mode

I can check and uncheck the checkbox to change the value of value to verify the the value is reactive, and <select> will get updated accordingly.

Besides that, I exported spread, so that I can change the object to something object to contain value, eg: { value: [] }, and see how <select> will update accordingly. Make sure that our fix not just work with value attribute, and also when the value is spreaded into <select>.

You may think that we are familiar with our fix, we know what it will fix, what it will not fix, do we need think up and write all the edge cases?

Well, I think you should. Future you will thank the present you when he encounter a fail test, that just mean his change may have an unintentional regression change. If you don’t have the test case, the future you will never know what edge case he didn’t accounted for.

Runtime test cases are added into test/runtime/samples/. Each folder represent 1 test case. Inside the folder, the component to be tested is named App.svelte, and the test case is written _config.js.

_config.js default exports a object:

_config.js

export default {
  // initial props to passed to the component
  props: { /*...*/ },
  // initial rendered html
  html: ``,
  // test case
  async test({ assert, target }) {
    // you can test the behavior of the component here
  },
};
Enter fullscreen mode Exit fullscreen mode

An example of test case of unchecking the checkbox, and verify <select> value get updated

export default {
    // ...
    async test({ assert, target, window }) {
    // find the element
        const [input1, input2] = target.querySelectorAll("input");
        const select = target.querySelector("select");
        const [option1, option2] = select.childNodes;

    // uncheck the checkbox
        const event = new window.Event("change");
        input1.checked = false;
        await input1.dispatchEvent(event);

    // verify the component updated correctly
        const selections = Array.from(select.selectedOptions);
        assert.equal(selections.length, 1);
        assert.ok(!selections.includes(option1));
        assert.ok(selections.includes(option2));
    },
};
Enter fullscreen mode Exit fullscreen mode

To run only this test, so that we can focus on ensuring the test case pass, we can set solo: true:

export default { solo: true };
Enter fullscreen mode Exit fullscreen mode

Quick tip: running npm run test will build Svelte code first before executing the test. If you are like me, running npm run dev on the background, Svelte code is build on every code change. So, npm run quicktest would allow you to skip the pretest build, and run the test suite immediately.

With the test, I realised that I didn’t handle the case when the value is updated.

So I guess what I needed to do is to add the same code in the p() (update) method too!

add_spread_attributes(block: Block) {
  // ...
  if (this.node.name === 'select') {
    block.chunks.mount.push(
      b`@select_options(${this.var}, ${data}.value);`
    );
    block.chunks.update.push(
      b`@select_options(${this.var}, ${data}.value);`
    );
  }
}
Enter fullscreen mode Exit fullscreen mode

Well, of course in this way, the select_options get executed unconditionally whenever any variable is updated.

I need to make sure that the select_options(...) inside the p() method get executed only when the value of value changes, and also probably when spread changes too, because it could potentially override the value of value.

If you’ve read Compile Svelte in your Head - Bitmask in Svelte, you know that Svelte uses bitmask to check any variable changes.

How do I know what is the bitmask to use in this case, well I dont have to.

I can use renderer.dirty(dependencies) to help me with that:

add_spread_attributes(block: Block) {
  // ...
  if (this.node.name === 'select') {
    const dependencies = [...];
    block.chunks.mount.push(
      b`@select_options(${this.var}, ${data}.value);`
    );
    block.chunks.update.push(
      // block.renderer.dirty(...) will give me `dirty & bitmask`
      b`if (${block.renderer.dirty(dependencies)}) @select_options(${this.var}, ${data}.value);`
    );
  }
}
Enter fullscreen mode Exit fullscreen mode

Next, I need to figure out what are the dependencies to be included. In this particular case, the dependencies of all attributes have to be taken consideration, because it is hard to tell which one would be eventually applied due to the spread attribute.

add_spread_attributes(block: Block) {
  // ...
  if (this.node.name === 'select') {
    const dependencies = new Set(); for (const attr of this.attributes) { for (const dep of attr.node.dependencies) { dependencies.add(dep); } } block.chunks.mount.push(
      b`@select_options(${this.var}, ${data}.value);`
    );
    block.chunks.update.push(
      b`if (${block.renderer.dirty(dependencies)}) @select_options(${this.var}, ${data}.value);`
    );
  }
}
Enter fullscreen mode Exit fullscreen mode

After a few tweaks, finally I passed all my test cases, and its time to create a pull request!

Submitting the fix

Before pushing the fix to remote, it is important to make sure that all the lints and typescript definitions are correct. You can run npm run lint --fixed for linting, and npm run tsd to generate typescript definition.

If you are unsure on how to create a pull request, you can check out How to make your first pull request on GitHub.

I pushed my branch and created a Pull Request to Svelte, and now I am waiting for feedback and for it to get merged.

Svelte is not maintained by full-time maintainers, everyone has their full-time job, so please be patient and be nice.

If you wish to learn more about Svelte, follow me on Twitter.

If you have anything unclear about this article, find me on Twitter too!

Top comments (0)

Read next

dmitry_bas profile image

Using SCP with ssh2 in Node.js Applications without SFTP

Dmitry Bas -

amitkumar13 profile image

Memory Leak Detection in React Native with LeakCanary

Amit Kumar -

whiskeygerman profile image

Web canvas with generative neural networks with elements of visual programming.

German Whiskey -

siisee11 profile image

Langgraph - Human In the Loop

Jaeyoun Nam -