sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
77.51k stars 4.04k forks source link

A simple component without script tag generates broken code when compiled #11945

Closed kran6a closed 1 day ago

kran6a commented 1 month ago

Describe the bug

I have the following dumb svelte 5 (5.0.0-next.151) component (src/Index.svelte):

<h1>AAA</h1>

Along with the following vite.config.ts:

import type {RollupOptions} from "rollup";
import {defineConfig} from 'vite';
import {svelte} from '@sveltejs/vite-plugin-svelte';

const ROLLUP_OPTIONS = {
    output: {
        format: 'esm',
        preserveModules: false,
        entryFileNames: 'index.js',
        esModule: true
    },
} satisfies RollupOptions;

export default defineConfig({
    mode: 'development',
    build: {
        rollupOptions: ROLLUP_OPTIONS,
        lib: {
            formats: ['es'],
            entry: './src/Index.svelte'
        },
        target: 'esnext',
        emptyOutDir: true,
        outDir: './dist',
        minify: false,
    },
    plugins: [svelte()]
});

It produces the following code:

// generated during release, do not modify

const PUBLIC_VERSION = '5';

if (typeof window !== 'undefined')
    // @ts-ignore
    (window.__svelte ||= { v: new Set() }).v.add(PUBLIC_VERSION);

const TEMPLATE_USE_IMPORT_NODE = 1 << 1;

// Store the references to globals in case someone tries to monkey patch these, causing the below
// to de-opt (this occurs often when using popular extensions).
var is_array = Array.isArray;

/** @type {null | import('#client').Effect} */
let current_effect = null;

/** @param {string} html */
function create_fragment_from_html(html) {
    var elem = document.createElement('template');
    elem.innerHTML = html;
    return elem.content;
}

/**
 * @template {import("#client").TemplateNode | import("#client").TemplateNode[]} T
 * @param {T} dom
 * @param {import("#client").Effect} effect
 */
function push_template_node(
    dom,
    effect = /** @type {import('#client').Effect} */ (current_effect)
) {
    var current_dom = effect.dom;
    if (current_dom === null) {
        effect.dom = dom;
    } else {
        if (!is_array(current_dom)) {
            current_dom = effect.dom = [current_dom];
        }

        if (is_array(dom)) {
            current_dom.push(...dom);
        } else {
            current_dom.push(dom);
        }
    }
    return dom;
}

/**
 * @param {string} content
 * @param {number} flags
 * @returns {() => Node | Node[]}
 */
/*#__NO_SIDE_EFFECTS__*/
function template(content, flags) {
    var use_import_node = (flags & TEMPLATE_USE_IMPORT_NODE) !== 0;

    /** @type {Node} */
    var node;

    return () => {

        if (!node) {
            node = create_fragment_from_html(content);
            node = /** @type {Node} */ (node.firstChild);
        }

        var clone = use_import_node ? document.importNode(node, true) : node.cloneNode(true);

        push_template_node(
            /** @type {import('#client').TemplateNode} */ (clone)
        );

        return clone;
    };
}

/**
 * Assign the created (or in hydration mode, traversed) dom elements to the current block
 * and insert the elements into the dom (in client mode).
 * @param {Text | Comment | Element} anchor
 * @param {DocumentFragment | Element} dom
 */
function append(anchor, dom) {
    // We intentionally do not assign the `dom` property of the effect here because it's far too
    // late. If we try, we will capture additional DOM elements that we cannot control the lifecycle
    // for and will inevitably cause memory leaks. See https://github.com/sveltejs/svelte/pull/11832

    anchor.before(/** @type {Node} */ (dom));
}

var root = template(`<h1>AAA</h1>`);

function Index($$anchor) {
    var h1 = root();

    append($$anchor, h1);
}

export { Index as default };

When I import the generated js code and try to mount it on a DOM node it does not work as I get Uncaught (in promise) TypeError: Cannot read properties of null (reading 'dom')

This error happens because it does var current_dom = effect.dom; where effect is default parameter value (current_effect), which is assigned null at the top of the file and is never reassigned.

Reproduction

Create the Index.svelte and vite config described above and run vite build

Logs

No response

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (12) x64 AMD Ryzen 5 3600XT 6-Core Processor
    Memory: 20.82 GB / 63.93 GB
  Binaries:
    Node: 22.2.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.7.0 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.1.1 - C:\Program Files\nodejs\pnpm.CMD
    bun: 1.1.12 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (125.0.2535.85)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    rollup: ^4.18.0 => 4.18.0
    svelte: 5.0.0-next.151 => 5.0.0-next.151

Severity

annoyance

Conduitry commented 1 month ago

Please provide a complete reproduction as a clonable repo - this is especially important in this case as you're using your own build process.

kran6a commented 1 month ago

https://github.com/kran6a/svelte-compile-bug-repro Just install the deps and run the start script. It will build the "inner" app (dist/index.js, the one with the bug) then the "outer" app and then start a dev server.

denlukia commented 3 weeks ago

TLDR: Creating a default Vite project with --template svelte-ts + updating it to Svelte 5 produces a broken app.

I wanted to try to understand all the work that Svelte 5 does to render and update a counter by going with a debugger step-by-step through a basic app. I initialised a basic project with pnpm create vite --template svelte-ts, updated Svelte from 4 to 5 (next.155), replaced App.svelte contents with default REPL counter example, pnpm build, pnpm preview and also got the same error in push_template_node: Cannot read properties of null (reading 'dom').

I've also added minify:false to Vite, but using default Vite config will lead to same error.

The OP already described the problem pretty well but I'll reiterate:

function push_template_node(dom, effect = (
  /** @type {import('#client').Effect} */
  current_effect
)) {
  var current_dom = effect.dom;
  if (current_dom === null) {
    effect.dom = dom;
  } else {
    if (!is_array(current_dom)) {
      current_dom = effect.dom = [current_dom];
    }
    if (is_array(dom)) {
      current_dom.push(...dom);
    } else {
      current_dom.push(dom);
    }
  }
  return dom;
}

We have created ourselves a too optimistic typing here

  /** @type {import('#client').Effect} */
  current_effect

because current_effect is actually initialised with null: https://github.com/sveltejs/svelte/blob/95d07de7f9711855295c3b0ce8adcada10981b12/packages/svelte/src/internal/client/runtime.js#L90-L93

dummdidumm commented 1 day ago

The build config is incorrect. You need to externalize all svelte/* imports (and the svelte import itself). You can do that by adding the following to your rollup config:

  external: ["svelte", /^svelte\/.+/],

Without this, a copy of the runtime is added to the build output. That means there are now two runtime versions, where only one for all should be used.