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

Server preload #31

Closed
wants to merge 3 commits into from
Closed

Server preload #31

wants to merge 3 commits into from

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Sep 4, 2020

@lukeed
Copy link
Member

lukeed commented Sep 4, 2020

Cool! A couple of quick comments:

  1. I know you want to steer module naming towards Multiple context=module Scripts #27 only, but module server doesn't have a direct tie-in to ssr output. The purpose of Multiple context=module Scripts #27 is only to ensure (or allow the opportunity to ensure) that the entire module exported interface is expecting the same environment. It'd feel weird to have module.default be set through generate: ssr|dom and have module.<...rest> be set through module client|server. I would think that the same terms/vocab should be used.

  2. In regard to:

    it's very easy to treat your web app as an API — just slap a .json on the end of a URL and you're done. We'd lose that.

    I don't think that we have to lose it. I know a lot of people are fond of this approach – and for simple endpoints, it can work really well. Also, tying in this:

    Paradoxically, it makes Sapper both easier and harder to learn

    I feel like we may be able to why-not-both.gif 🤷 What if we added the ability to:

    • export an API method from a module:ssr block to use that as its preload:

      <script context="module:ssr">
        // file: blog/[slug].svelte
        //=> [slug] matches, can wire up automatically
        export { get as preload } from './[slug].json.js';
      </script>
    • export an API method from a module:dom block & auto-generate a fetch request:

      <script context="module:dom">
        // file: blog/[slug].svelte
        //=> [slug] matches, can wire up automatically
        export { get as preload } from './[slug].json.js';
      </script>
      
      <!-- becomes -->
      <script context="module:dom">
        export function preload(req) {
          return fetch(`/blog/${req.params.slug}.json`).then(r => r.json());
        }
      </script>
    • if only a module:ssr is defined, auto-generate a module:dom preloader using fetch caller:

      <script context="module:ssr">
        // file: blog/[slug].svelte
        import sql from 'postgres';
      
        export async function preload(req) {
          const [item] = await sql`select * from posts where slug = ${req.params.slug} limit 1`;
          return item;
        }
      </script>
      
      <!-- becomes -->
      
      <script context="module:ssr">
        // ~~ effectively the same ~~ see below
        export { get as preload } from './[slug].json.js';
      </script>
      
      <script context="module:dom">
        export function preload(req) {
          return fetch(`/blog/${req.params.slug}.json`).then(r => r.json());
        }
      </script>

      – We would take the module:ssr contents (as is) and also auto-generate a would-be get method at blog/[slug].json.js. The code is copy-pasta'd as is, and can just be a virtual file. Perhaps this is too risky. At the very least, it couldn't be done if get on blog/[slug].json.js already exists and was just ignored for this file.

This 2nd point is somewhat where my mind was going when you mentioned this "auto-generate DOM stuff" idea in our last meeting. However, seeing this RFC sparked some better ideas/details that may be more possible than what I was originally thinking.

Will pass-through again tomorrow for more feedback on what's presented.

@Conduitry
Copy link
Member

Something I mentioned on Discord but should probably have gone here: The Accept header with the same URL for the endpoint and the page would be difficult with exported apps.

Would we be able to make the URL of the implicit endpoint an implementation detail, so we're free to adjust it as we see fit? If someone really wants to be able to access it in a more public way, they can just create a server route like they do now.

Separately, does this really need the multiple <script context=module> feature? Can this just be a different module-level export from the existing <script context=module>? Is the main reason for having different module scripts so that we can use static rather than dynamic imports for things that need to be loaded only on the server?

@Rich-Harris
Copy link
Member Author

module server doesn't have a direct tie-in to ssr output

yeah, that was needlessly confusing. Changed to module ssr and module dom. I guess my main contribution here was to suggest that treating contexts as 'tags' (["module", "ssr"]) rather than specific things ("module:ssr") potentially gives us more composability in future if we wanted to add new contexts like "module ssr test" and "dom test" or other as-yet-unimagined things (context="dev" and context="prod" or whatever). But I haven't thought this through beyond what's presented here.

  • export an API method from a module:dom block & auto-generate a fetch request:

I'm a little confused by this part — it looks like the module:dom block is re-exporting from a server route, which suggests that the database client/fs/whatever is getting imported into the client bundle?

The Accept header with the same URL for the endpoint and the page would be difficult with exported apps.

Yeah, this is... a good point. In which case...

Would we be able to make the URL of the implicit endpoint an implementation detail, so we're free to adjust it as we see fit?

...perhaps the default is to tack on the .json after all, as though the [slug].json.js file were being generated on our behalf. (Unless the developer already added their own, that is. Sort of like how if you don't supply a _layout.svelte, Sapper provides one that just contains <slot></slot>.) We could use whatever path we wanted — /xyz123.json?x=[slug] — but I see no reason not to use the same path as the page itself.

Is the main reason for having different module scripts so that we can use static rather than dynamic imports for things that need to be loaded only on the server?

Partly that. It side-steps the (real) confusion people experience when they try to use packages in preload that only work in a Node environment, and static imports are just nicer to work with than dynamic ones (and more tree-shakeable, which obviously matters if you're trying to minimise your server's footprint).

But it also means the bundler isn't creating vestigial chunks for things that will never be dynamically imported in the browser — just because serverPreload never runs in the client here...

<script context="module">
  export async function serverPreload(page) {
    const sql = await import('postgres');
    // ...
  }
</script>

...doesn't mean Rollup can skip resolving/analysing/emitting a chunk for postgres in the client app. (Of course you could wrap the whole thing in process.browser, and then assuming everything is correctly configured, the dynamic import should get DCE'd before doing any of that, but the ergonomics of checking you're in a server environment in a function called serverPreload aren't great.)

@Rich-Harris
Copy link
Member Author

I suppose I should mention that this is possible to do in userland with a preprocessor. Quick hacky version:

Component.svelte
<script context="module" ssr>
  export async function load(page) {
    const article = await get_post_somehow(page.params.slug);

    if (article) {
      return { article };
    } else {
      this.error(404, {
        message: 'Not found'
      });
    }
  }
</script>

<script>
  export let article;
</script>

<h1>{article.title}</h1>
transform.js
const fs = require('fs');
const { preprocess } = require('svelte/compiler');

const source = fs.readFileSync('Component.svelte', 'utf-8');

const get_preprocess = dom => {
  return {
    markup: ({ content }) => {
      const scripts = [];

      const re = /<script([^>]*?)>([\s\S]+?)<\/script>/gm;
      let match;
      while (match = re.exec(content)) {
        const attrs = match[1].split(' ')
          .map(str => str.trim())
          .filter(Boolean)
          .map(str => {
            const [name, quoted_value] = str.split('=');
            const value = quoted_value
              ? quoted_value.replace(/^['"]/, '').replace(/['"]$/, '')
              : true;

            return { name, value };
          })
          .reduce((attrs, { name, value }) => {
            attrs[name] = value;
            return attrs;
          }, {});

        scripts.push({ attrs, content: match[2] });
      }

      const ssr_module_script = scripts.find(s => s.attrs.context === 'module' && s.attrs.ssr);
      const dom_module_script = scripts.find(s => s.attrs.context === 'module' && s.attrs.dom);

      console.log(scripts);

      let i = 0;

      if (dom) {
        const implicit_dom_module_script = dom_module_script ? '' : `
        <script context="module">
          export { preload } from '@sapper/internal';
        </script>`.replace(/^\t{4}/gm, '');

        return {
          code: content.replace(re, (_, attrs, content) => {
            const script = scripts[i++];

            if (script === ssr_module_script) {
              console.log('!!!!', script)
              return _.replace(/\S/g, ' ');
            }

            return _;
          }) + implicit_dom_module_script
        }
      } else {
        return {
          code: content.replace(re, (_, attrs, content) => {
            const script = scripts[i++];

            if (script === dom_module_script) {
              return _.replace(/\S/g, ' ');
            }

            return _;
          })
        };
      }
    }
  };
};

async function main() {
  const ssr_processed = await preprocess(source, get_preprocess(false));
  const dom_processed = await preprocess(source, get_preprocess(true));

  fs.writeFileSync('Component.ssr.svelte', ssr_processed.code);
  fs.writeFileSync('Component.dom.svelte', dom_processed.code);
}

main();
Component.dom.svelte
                             
                                    
                                                             

                  
                         
            
                       
                            
         
     
   
         

<script>
  export let article;
</script>

<h1>{article.title}</h1>
<script context="module">
  export { preload } from '@sapper/internal';
</script>
Component.ssr.svelte
<script context="module" ssr>
  export async function load(page) {
    const article = await get_post_somehow(page.params.slug);

    if (article) {
      return { article };
    } else {
      this.error(404, {
        message: 'Not found'
      });
    }
  }
</script>

<script>
  export let article;
</script>

<h1>{article.title}</h1>

@lukeed
Copy link
Member

lukeed commented Sep 4, 2020

I'm a little confused by this part — it looks like the module:dom block is re-exporting from a server route, which suggests that the database client/fs/whatever is getting imported into the client bundle?

There's a becomes block at the end of it. The idea is that you're telling sapper "use this API method for preloading". That export never makes it into the client output. Instead, Sapper sees that it's a *.json.js file and just generates a fetch() call pointing to that API route. (This is the becomes block) That's doable because both files are using [slug] to identify. And of course, if you're doing something more custom, you would do it yourself instead of this re-export shortcut

@bfanger
Copy link

bfanger commented Sep 6, 2020

An alternative approach to reducing the boilerplate:

If the filesystem looks like this:

blog/[slug].svelte
blog/[slug].json.js

And the blog/[slug].svelte doesn't export an preload method, use a generic implementation that works with following convention:

The request format is standarized: The blog/[slug].json.js receives the [slug] section of the path as query parameter: ?slug=what-is-new

The json response matches the props of the component.

{
  "id": 135,
  "title": "What's new in Sapper"
}
<script>
  export let id;
  export let title;
</script>

When the response is not an 200 OK render the src/routes/_error.svelte page.
(Passing the response as the error)

Pro's

  • Doesn't mix serverside and clientside code (less chance of accidentaly leaking a server key)
  • No (svelte) compiler changes needed.
  • Still works for SSG sites (if you don't use query params)
  • Developer friendly (errors are managed for you, no new api, but easy to remember conventions)
  • Allows supporting different data formats in the future: blog/[slug].yaml.js or blog/[slug].devalue.js
  • Compatible with an module="context server/client" approach in the future.

Con's

  • Doesn't mix serverside and clientside code (Multiple source files needed for a single page.)
  • The code inside the routes/_error.svelte now also needs to support response objects.
  • Doesn't take authentication into account, or any other request/response manipulation for that matter.
  • When you specificy a preload you'd don't have access to the default behavior.
  • No separation between buildtime data and runtime data.

Some of the cons could be solved by exposing hooks and middleware into the generic preload function: Setting headers, redirect behavior on 403, etc. But that gets complex fast, and at that point you'd be better off writing your own createPreload function that generates preload functions.

Adding a preloadUrl: "/blog/what-is-new.json" to the page object based on the filesystem and params would be helpful.

@ajbouh
Copy link

ajbouh commented Sep 18, 2020

An additional unresolved question here is: how should updates to the session store interact with server preloads?

Today in sapper the preload function is called every time the session store is updated. The current behavior, when combined with more explicit server-side preload machinery, would probably be extra surprising to developers using sapper.

I don't want to advocate for broadening the scope of this RFC unnecessarily, but I am curious to understand how this RFC is intended to interact with the existing preload/session store dynamic.

@Glench
Copy link

Glench commented Dec 29, 2020

Hey! I'm not part of the dev team and just a happy user of Svelte, but I thought I'd share for reference a little framework I independently made for myself to use in a real system that feels pretty nice and is related to this RFC. It's designed for use with Node. Sorry if this comment isn't appropriate!

Basically I hacked together the Svelte compiler to make files like this work:

<!-- routes/index.svelte -->
<script context="node">
  import db from '../db';
  export async function get(req, abort, redirect) {
      const user = await db.User.findOne({where: {id: req.query.user_id}});
      return {user}
  }
</script>

<script>
  export let user;
</script>

<h1>Hi {user.name}!</h1>

The part in the first <script> tag is stripped out on the client version. The data that is returned from the get function is automatically JSONified, and either sent to the client (automatically on AJAX POST requests!) or used to server-render the HTML. I'm not proposing that any of this be the preferred way it should be in Svelte/Sveltekit, just that it's a way I've found productive.

So far it's great — I love the tighter coupling between server and UI (just like Svelte takes previously-decoupled HTML/CSS/JS and puts it together). It makes creating and tracing the relationship between the client and server much more straightforward for me. I also like how it's very easy to tell what code and data will be shown to the client and what won't (being paranoid about data exposure in my small business). It's a little tedious-feeling to have to define variables multiple times (return {user} in the server code, export let user in the component setup) — it feels like the user variable should just automatically show up in my Svelte component, but otherwise it's fun. So far I have yet to find a case where I wanted the same library in the server and client-side code but I'm sure I'll run into that problem eventually. Hope this helps!

@aral
Copy link

aral commented Apr 30, 2021

@Glench is your code available somewhere? I’d like to look into this too. Might be a good way to integrate with Site.js (especially re: JSDB).

@Glench
Copy link

Glench commented Apr 30, 2021

Here's a probably out-of-date version: https://github.com/Glench/full-stack-svelte-prototype/

@madeleineostoja
Copy link

Is this still under consideration in the context of sveltekit? I think it's an essential feature, for all the reasons outlined in the RFC. Without it a simple CMS-backed website becomes pretty cumbersome, and it's not intuitive for people coming from a NextJS environment that load runs on the client as well and all the repurcussions of that (serverside dep bloat, clientside route change loading, etc), even though its documented.

@teds31
Copy link

teds31 commented Jun 14, 2021

Is this still under consideration in the context of sveltekit? I think it's an essential feature, for all the reasons outlined in the RFC. Without it a simple CMS-backed website becomes pretty cumbersome, and it's not intuitive for people coming from a NextJS environment that load runs on the client as well and all the repurcussions of that (serverside dep bloat, clientside route change loading, etc), even though its documented.

running into a few issues with prismic right now building a related articles component. looking forward to this.

@Rich-Harris
Copy link
Member Author

I'm going to close this as we recently added page endpoints to SvelteKit, which solve this problem more elegantly

@Rich-Harris Rich-Harris closed this Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants