sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.73k stars 1.94k forks source link

Explicit SSR Imports #905

Closed DrewRidley closed 3 years ago

DrewRidley commented 3 years ago

Is your feature request related to a problem? Please describe. In short, the issue here is caused by how SSR is generated and bundled. When import client side libraries such as firebase or js-cookie I have found that the site does not run because the client libraries are still being imported into the server runtime resulting in SSR errors.

Describe the solution you'd like Ideally there would be a vite extension that can interpret imports accordingly such that the imports can be top level like they are in sapper. From my experience I did not have this issue when using Sapper, only with sveltekit.

Describe alternatives you've considered One possible alternative would be to have a config file where you can explicitly declare which imports are server and which imports are client side. Not really sure how the sapper stack handled it but I could not reproduce a similar issue with sapper.

How important is this feature to you? This feature is not direly important, but it is a quality of life feature and most developers might be confused by the inability for them to easily import modules at the top level of their script blocks without causing various warnings or errors.

DrewRidley commented 3 years ago

To clarify,

    import { goto } from '$app/navigation'; 
    import { browser } from '$app/env';

    if (browser) {
        import firebase from '$lib/fb.js';
        import Cookies from 'js-cookie';
    }

The recommended approach which is to use a conditional with the browser statement does not appear to work as it gives an error about top level imports.

'import' and 'export' may only appear at the top level.

I scoured the sapper github and noticed a similar issue which was solved with a later version of rollup, but sveltekit does not depend on rollup so I am not really sure how to avoid this compiler error.

benmccann commented 3 years ago

You can do a dynamic import in onMount. See https://kit.svelte.dev/docs#troubleshooting-server-side-rendering

DrewRidley commented 3 years ago

I spent a few hours communicating with people on the discord about the issue and its a pretty significant migration to require all users to replace top level imports with sophisticated and ugly dynamic imports when coming from sapper.

import lib from 'lib'

onMount() {
   useLib();
}

the following example worked fine with sappers bundler, but not with vite. Instead, users are expected to learn how to use dynamic imports without any documentation or clarification as to why.

let lib;
onMount() {
   if (browser) {
    lib = (await import('lib')).default;
  }
}

perhaps these examples can shed some light as to why this issue should be investigated further. As a side note, I did a bunch of research and was not entirely sure whether or not vite can do treeshaking of dynamic imports in this format. My initial findings led me to believe that treeshaking is not performed when its used this way, which could have an adverse affect on the bundle size of some applications that use large client side libraries (ie, RTC, firebase).

Let me know if you have any suggestions or comments, @benmccann . Thanks for your help.

benmccann commented 3 years ago

I've proposed a solution for this type of problem over in https://github.com/sveltejs/kit/issues/754

DrewRidley commented 3 years ago

I am not necessarily referencing a true SPA scenario. I was thinking something more along the lines of sapper. I still want SSR to be enabled, but I would like certain imports that are client-only to still feel natural. I can look into the how the vite plugin works and its possible this can be adapted to be added. I understand that vite is very different from rollup so I dont know how feasible it is.

benmccann commented 3 years ago

I guess you could automatically wrap every onMount call with if (browser) in a custom preprocessor. You'd still need the dynamic import in that case though. I'm not sure this is something we'd want to add to the default vite plugin at this point. It'd probably be better to keep it separate and see if there's enough demand for it before we'd want to do something like that on a more official basis

But I'd say your best bet would be to try https://modularfirebase.web.app/ and hope it might support being loaded on the server-side more easily than the standard Firebase libraries (which were a huge source of pain even in Sapper even if it's a bit worse in SvelteKit) and if not try to give feedback to the Firebase team that helps them improve those libraries (e.g. using fetch instead of XMLHttpRequest, etc.)

benmccann commented 3 years ago

@DrewRidley thanks for the idea. While I wasn't so excited about the prospect of doing this with a preprocessor, I think we could do this directly in the Svelte compiler. We've filed an issue for this over in https://github.com/sveltejs/svelte/issues/6372

jthegedus commented 3 years ago

But I'd say your best bet would be to try modularfirebase.web.app and hope it might support being loaded on the server-side more easily than the standard Firebase libraries (which were a huge source of pain even in Sapper even if it's a bit worse in SvelteKit) and if not try to give feedback to the Firebase team that helps them improve those libraries (e.g. using fetch instead of XMLHttpRequest, etc.)

Just to reduce confusion on this topic, Firebase have distinct libraries for Browser vs Server which share some API surface. The modular (v9) rewrite of these SDKs is in different states:

The differences in the APIs depends on the products being used, with calls to services from the Server (Cloud Function is the common env for this) bypassing the Security Rules model of Firebase. Given that, take care using Firestore from the Browser SDK in SvelteKit Endpoints or SSR components as your Sec Rules are being bypassed.

cupcakearmy commented 3 years ago

Has anyone got it to work in svelte kit?

I constantly get IDBIndex is not defined event with

import preprocess from 'svelte-preprocess'
import adapter from '@sveltejs/adapter-node'

/** @type {import('@sveltejs/kit').Config} */
const config = {
  preprocess: preprocess(),

  kit: {
    adapter: adapter(),
    target: '#svelte',
    ssr: false,
    prerender: {
      enabled: false,
    },
  },
}

export default config

Is there something I'm missing? dynamic importing the library in an onMount does not seem to work.

jthegedus commented 3 years ago

@cupcakearmy It depends on the Firebase modules you are using. firebase/firestore, which most people are using, doesn't have a dependency on IDBIndex so aren't facing this issue.

~This is feedback the Firebase team would like to receive. I have observed the IDBIndex error you report when trying to use the firebase/performance module.~

~The vite.optimizeDeps.include[] config doesn't resolve the issue.~

Also of note, the latest npm v9 SDK has changed from firebase@exp to firebase@beta


Update: it works when you dynamically import the library.

// $lib/firebaseClient.js
import { initializeApp } from "firebase/app";
import { getPerformance } from "firebase/performance";

const app = initializeApp({
    ...
});

function performance() {
    return getPerformance(app);
}

export { performance };
<script>
    import { browser } from "$app/env";
    import { onMount } from "svelte";

    // this does not work
    // import { performance } from "$lib/firebaseClient"

    let perf;

    onMount(async () => {
        if (browser) {
            // this works
            const firebaseClient = await import("$lib/firebaseClient");
            perf = firebaseClient.performance(); // this initialises the perf-mon module.
        }
    })  
</script>

The key thing here is that firebaseClient and thus, firebase/performance is not imported and evaluated until the required window.db is available.

An alternative is to push the await import() into firebaseClient ```js // $lib/firebaseClient.js import { initializeApp } from "firebase/app"; const app = initializeApp({ ... }); async function performance() { const perf = await import("firebase/performance"); return perf.getPerformance(app); } export { performance }; ``` ```svelte ```
cupcakearmy commented 3 years ago

@jthegedus I got it work now. The issue that I was not realizing was that dynamic for firebase don't work in dev mode, only in the actual builds for some reason. Thanks!

jthegedus commented 3 years ago

@jthegedus I got it work now. The issue that I was not realizing was that dynamic for firebase don't work in dev mode, only in the actual builds for some reason. Thanks!

Curious, it works in Dev and Prod for me

cupcakearmy commented 3 years ago

The quirks of working with beta software I guess :) But I love svelte kit too much to wait 😆 It's the best thing that happened to web development in a looooing time 🥰

sheecegardezi commented 1 year ago

i dont think this is resolved!

lucidNTR commented 1 year ago

the issue seems to have been accidentally closed by an unrelated PR. the dynamic imports break bundling and also slow down app startup.

benmccann commented 1 year ago

I'm not sure that there's anything we could change in SvelteKit, but our current design for Svelte 5 will make it easier and more natural to deal with client-side only code, so stay tuned!

carstenjaksch commented 12 months ago

I have a similar issue, but with a Node module (jsdom) that should only run on the server. On the client, I want to use the corresponding browser APIs.

Asked a question about that: https://github.com/sveltejs/kit/discussions/11090

FeldrinH commented 5 months ago

I'm not sure that there's anything we could change in SvelteKit, but our current design for Svelte 5 will make it easier and more natural to deal with client-side only code, so stay tuned!

@benmccann What is this new design in Svelte 5 that you mentioned? I took a look at the Svelte 5 RC documentation and release notes and could not figure out what part of the changes makes dealing with client-side only code easier.