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

ESM Modules: ssrTransform blindly overwrites all functions named as an import instead of checking for a locally scoped function with the same name #4306

Closed
6 tasks done
btakita opened this issue Jul 18, 2021 · 3 comments · Fixed by #5376
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)

Comments

@btakita
Copy link

btakita commented Jul 18, 2021

Describe the bug

See: sveltejs/kit#1947

ssrTransform replaces all subscribe references defined within writable with __vite_ssr_import_0__.subscribe instead of using the subscribe function defined within writable.

Due to ssrTransform https://github.com/vitejs/vite/blob/main/packages/vite/src/node/ssr/ssrTransform.ts#L189,
writable in store/index.mjs is transpiled to become:

function writable(value, start = __vite_ssr_import_0__.noop) {
    let stop;
    const subscribers = [];
    function set(new_value) {
        if (__vite_ssr_import_0__.safe_not_equal(value, new_value)) {
            value = new_value;
            if (stop) { // store is ready
                const run_queue = !subscriber_queue.length;
                for (let i = 0; i < subscribers.length; i += 1) {
                    const s = subscribers[i];
                    s[1]();
                    subscriber_queue.push(s, value);
                }
                if (run_queue) {
                    for (let i = 0; i < subscriber_queue.length; i += 2) {
                        subscriber_queue[i][0](subscriber_queue[i + 1]);
                    }
                    subscriber_queue.length = 0;
                }
            }
        }
    }
    function update(fn) {
        set(fn(value));
    }
    function subscribe(run, invalidate = __vite_ssr_import_0__.noop) {
        const subscriber = [run, invalidate];
        subscribers.push(subscriber);
        if (subscribers.length === 1) {
            stop = start(set) || __vite_ssr_import_0__.noop;
        }
        run(value);
        return () => {
            const index = subscribers.indexOf(subscriber);
            if (index !== -1) {
                subscribers.splice(index, 1);
            }
            if (subscribers.length === 0) {
                stop();
                stop = null;
            }
        };
    }
    return { set, update, subscribe: __vite_ssr_import_0__.subscribe };
}

Reproduction

https://github.com/btakita/sveltekit-repro-subscribe-issue

svelte.config.js is forced to be an esm module by using a top-level await: https://github.com/btakita/sveltekit-repro-subscribe-issue/blob/main/svelte.config.js#L4

svelte/store writable is transpiled where subscribe is replaced by vite_ssr_import_0.subscribe https://github.com/btakita/sveltekit-repro-subscribe-issue/blob/main/src/hooks/index.ts

System Info

System:
    OS: Linux 5.12 Arch Linux
    CPU: (16) x64 AMD Ryzen 7 5800X 8-Core Processor
    Memory: 33.55 GB / 62.78 GB
    Container: Yes
    Shell: 5.8 - /usr/bin/zsh
  Binaries:
    Node: 16.5.0 - ~/.config/nvm/versions/node/v16.5.0/bin/node
    Yarn: 1.22.10 - /usr/bin/yarn
    npm: 7.19.1 - ~/.config/nvm/versions/node/v16.5.0/bin/npm
  Browsers:
    Brave Browser: 91.1.26.77
    Chromium: 91.0.4472.164

Used Package Manager

npm

Logs

No response

Validations

@benmccann
Copy link
Collaborator

Here's the code in question: https://github.com/sveltejs/svelte/blob/ff6ce725bfdabba908690f4bae0b06a3f26da881/src/runtime/store/index.ts#L91

That gets compiled to store/index.mjs in the Svelte package:

import { noop, safe_not_equal, subscribe, run_all, is_function } from '../internal/index.mjs';

...

/**
 * Create a `Writable` store that allows both updating and reading by subscription.
 * @param {*=}value initial value
 * @param {StartStopNotifier=}start start and stop notifications for subscriptions
 */
function writable(value, start = noop) {
    let stop;
    const subscribers = [];
    function set(new_value) {
        if (safe_not_equal(value, new_value)) {
            value = new_value;
            if (stop) { // store is ready
                const run_queue = !subscriber_queue.length;
                for (let i = 0; i < subscribers.length; i += 1) {
                    const s = subscribers[i];
                    s[1]();
                    subscriber_queue.push(s, value);
                }
                if (run_queue) {
                    for (let i = 0; i < subscriber_queue.length; i += 2) {
                        subscriber_queue[i][0](subscriber_queue[i + 1]);
                    }
                    subscriber_queue.length = 0;
                }
            }
        }
    }
    function update(fn) {
        set(fn(value));
    }
    function subscribe(run, invalidate = noop) {
        const subscriber = [run, invalidate];
        subscribers.push(subscriber);
        if (subscribers.length === 1) {
            stop = start(set) || noop;
        }
        run(value);
        return () => {
            const index = subscribers.indexOf(subscriber);
            if (index !== -1) {
                subscribers.splice(index, 1);
            }
            if (subscribers.length === 0) {
                stop();
                stop = null;
            }
        };
    }
    return { set, update, subscribe };
}

There are two functions named subscribe in this code and it appears ssrTransform doesn't realize that and replaces them both

@Lenard-0
Copy link

Getting this issue. store.subscribe is not a function

@AlbertMarashi
Copy link

Getting issue as well when running a fork of svelte in my project.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants