vikejs / vike

🔨 Flexible, lean, community-driven, dependable, fast Vite-based frontend framework.
https://vike.dev
MIT License
4.3k stars 348 forks source link

V1 Design #578

Closed brillout closed 10 months ago

brillout commented 1 year ago

The vite-plugin-ssr@1.0.0 design in a nutshell:

// /pages/product/+config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  Page: './Page.vue',
  route: '/product/@id',
  onBeforeRender: './onBeforeRender.ts',
  onPrerender: './onPrerender.ts',
  prerender: true,
  ssr: false // render as SPA
} satisfies Config // `satisfies` is a new TypeScript 4.9 operator

I.e. +config.js replaces .page.js, .page.server.js, .page.client.js, and .page.route.js.

It also replaces _default.page.js as well as VPS configurations set in vite.config.js:

// /pages/+config.ts

import type { Config } from 'vite-plugin-ssr'

// Applies as default to all pages
export default {
  onRenderHtml: './config/onRenderHtml.tsx',
  onRenderClient: './config/onRenderClient.tsx',
  prerender: {
    partial: true // Currently lives in vite.config.js
  },
  ssr: true
} satisfies Config

VPS's architecture stays the same and therefore migration is relatively simple.

Succint syntax

Under consideration is the possibility to create new pages in a succint manner:

/pages/index/+Page.js
/pages/about/+Page.js
/pages/jobs/+Page.js

Here, a new +config.js file isn't needed each time a new page is created.

I'm currently leaning towards not supporting such succint syntax (in favor of "Single Config File", see next section). But I may reconsider if users want this.

Single Config File (aka Single Route File)

// /pages/+config.ts

import type { Config } from 'vite-plugin-ssr'

const marketingPagesConfig = {
  Layout: './layouts/LayoutMarketingPages.vue',
  ssr: true,
}
const adminPagesConfig = {
  title: 'Admin Panel',
  Layout: './layouts/LayoutAdminPages.vue',
  ssr: false
}

export default {
  // If `singleConfigFile: true` then only one `+` file is allowed (this file). If there is
  // anothoer `+` file, then VPS displays a warning.
  singleConfigFile: true,
  pages: [
    { ...marketingPagesConfig, Page: './LandingPage.vue', route: '/'        , title: 'Awesome Startup'                          },
    { ...marketingPagesConfig, Page:    './JobsPage.vue', route: '/jobs'    , title: "We're hiring!"                            },
    { ...marketingPagesConfig, Page:  './VisionPage.vue', route: '/vision'  , title: 'Our mission <3'                           },
    {     ...adminPagesConfig, Page:   './AdminPage.vue', route: '/admin'   , onBeforeRender:   './AdminPage-onBeforeRender.ts' },
    {     ...adminPagesConfig, Page: './AdminDbPage.vue', route: '/admin/db', onBeforeRender: './AdminDbPage-onBeforeRender.ts' }
  ]
} satisfies Config

The singleConfigFile enforces the entire app to be defined in that single +config.js file. This means that this single +config.js file represents the entire interface between VPS and your app.

Nested Views

(Aka "Nested Layouts", but "Nested Views" is a better name for this design.)

// /pages/product/+config.js

export default {
  Page: './Page.js',
  onBeforeRender: './onBeforeRender.js',
  route: '/product/@id',
  nested: [
    {
      route: '/product/@id/details',
      View: './DetailsView.js'
    },
    {
      route: '/product/@id/reviews',
      View: './ReviewsView.js',
      onBeforeRender: './onBeforeRenderReviewsView.js'
    }
  ]
}

This example doesn't use singleConfigFile (otherwise nested would be defined in /pages/+config.js instead).

Custom Exports

The V1 design requires Custom Exports to be registered.

// /pages/+config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  configDefinitions: [
    {
      name: 'title',
      env: 'SERVER_ONLY' // | 'CLIENT_ONLY' | 'CLIENT_AND_SERVER'
    }
  ]
} satisfies Config
// /pages/index/+title.ts

// The file is loaded only in Node.js. (At server run-time or at build-time while pre-rendering.)
export const title = 'Welcome to the V1 design'

More

The V1 design unlocks many more capabilities, in particular around building frameworks on top of VPS. See end of this comment.

This is a very exciting design. (In many subtle ways, you'll see when you'll use a VPS framework.)

Feedback

(Negative) feedback welcome.

Acknowledgement

🙏 @AaronBeaudoin for the fruitful conversation.

samuelstroschein commented 1 year ago

Treat the following as notes, not explicit feedback or requests.

singleConfigFile contradicts the nested example?

nested layout API via an array

using an array instead of a string for the Layout prop seems to enable very simple and intuitive nested layouts:

const adminPagesConfig = {
  title: 'Admin Panel',
  // the first layout is the outer layout
  Layout: [
     './layouts/RootLayout.vue',
     './layouts/LayoutAdminPages.vue',
     // renders 
     //   <RootLayout>
     //      <LayoutAdmingPages>
     //           {children}
     //       </LayoutAdminPages>
     //    </RootLayout>
  ]
}

nested pages that define additional layouts can be merged easily with parent.Layout.concat(child.Layout):

// parent
export default {
  // additional parameters
  Layout: ["./Layouts/RootLayout.tsx"]
}

// renders ["./Layouts/RootLayout.tsx"]
// page1
export default {
  // additional parameters
  Layout: [
       "./Layouts/Product.tsx", 
       "./Layouts/Info.tsx"
   ]
}

// renders parent.Layout.concat(page1.Layout) = 
// ["./Layouts/RootLayout.tsx", "./Layouts/Product.tsx", "./Layouts/Info.tsx"]
// page2 is a child of page1
export default {
  // additional parameters
  Layout: ["./Layouts/Sidenav.tsx"]
}

// renders parent.Layout.concat(page1.Layout).concat(page2.Layout) = 
// ["./Layouts/RootLayout.tsx", "./Layouts/Product.tsx", "./Layouts/Info.tsx", "./Layouts/Sidenav.tsx"]

the nested layout example seems unintuitive

Defining the layout for nested pages in a page config seems unintuitive. Either each page is responsible for defining its layouts, or one config that defines everything.

(I assume that /product/@id/details is a page too (if not why not?))

export default {
  // ...
  nested: [
    // why are layouts defined for sub-routes in a page config? 
    {
      route: '/product/@id/details',
      View: './DetailsView.js'
    },
  ]
}
brillout commented 1 year ago

@samuelstroschein

  1. If you use singleConfigFile then you define nested inside your single config file /pages/+config.js:

    // /pages/+config.js
    
    export default {
     pages: [
       {
         route: '/product/@id',
         Page: './product/ProductPage.js',
         nested: [
           {
             route: '/product/@id/details',
             View: './product/DetailsView.js'
           }
         ]
       }
     ]
    }

    Otherwise you define it in the page's config file /pages/product/+config.js:

    // /pages/product/+config.js
    
    export default {
     route: '/product/@id',
     Page: './ProductPage.js',
     nested: [
       {
         route: '/product/@id/details',
         View: './DetailsView.js'
       }
     ]
    }

    Both are equivalent.

  2. Are nested +config files auto-imported?

    All file paths are auto-imported (VPS knows in what environment and when to import them).

  3. Nested Views/Layouts are about parallelizing data fetching and that's why it needs to be built into the framework. Your proposed solution can already be achieved with VPS 0.4.
  4. Nested Views (note the naming "Nested Views" instead of Nested Layouts) aren't pages and don't behave like them. I believe the way Next.js, Nuxt and Remix does it is wrong. (Btw. nested views/layouts always have dedicated routes, see https://vite-plugin-ssr.com/layouts#nested-layouts.)

I've updated the RFC to better reflect this.

ArnaudBarre commented 1 year ago

I'm not doing any SSR right now so I can't give feedback on that side. Just taking the opportunity to suggest another syntax for config:

import type { Config } from 'vite-plugin-ssr'

export const config: Config = {
  // ...
}

This could avoid a lot of interop issues I think.

brillout commented 1 year ago

@ArnaudBarre 👍 From a TypeScript/user perspective I believe that would work equally well. I think it mostly boils down to a preference of having export const config vs export default. (It's reasonable to ask users to update their TS version. As for ESM/CJS compat it doesn't make a difference since VPS is always the only consumer of +config.js files.)

astahmer commented 1 year ago

regarding the ssr boolean option, where does that leave the current html-only (x.page.server.ts with no x.page.ts) rendering ?

so it would completely replace & deprecate the current {name}.page.{type}.ts convention ? fwiw the {name}.page.{type}.ts felt intuitive to me, maybe others too since it's quite common in current meta-framework (Rakkas/Next/Remix/Nuxt/etc) I suppose that the benefits are the same as Remix/tanstack router = knowing every route to avoid a fetching-cascade ?

anyway, it looks great, especially the custom exports ! congratz

brillout commented 1 year ago

@astahmer

HTML-only:

// +config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  configDefinitions: [
    {
      name: 'htmlOnly',
      handler(value) {
        if (typeof value !== 'boolean') throw new Error('Config htmlOnly should be a boolean')
        if (value) {
           return {
             name: 'Page',
             // Akin to .page.server.js
             env: 'SERVER_ONLY'
           }
        } else {
           return {
             name: 'Page',
             // Akin to .page.js
             env: 'CLIENT_AND_SERVER'
           }
        }
    }
  ]
} satisfies Config
// /pages/about/+config.js

export default {
  Page: './Page.jsx',
  htmlOnly: true
}
// /pages/about/Page.jsx

// This file is loaded only on the server

export default function() {
  return <>I'm a HTML-only page.</>
}

FYI, the idea long term is to have frameworks define such configs.

For example a VPS user is currently implementing a framework with island architecture. With V1 it works like this:

// node_modules/some-framework/+config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  configDefinitions: [{
    name: 'island',
    handler(value) {
      if (typeof value !== 'boolean') throw new Error('Config island should be a boolean')
      if (value) {
         return {
           clientEntries: ['./client-island-hydration.ts']
         }
      } else {
         return {
           clientEntries: ['./client-classic-hydration.ts']
         }
      }
    }
  }]
} satisfies Config
// /pages/about/+config.js

export default {
  Page: './Page.jsx',
  island: true
}
// /pages/about/Page.jsx

export default function() {
  return <>This page is island-hydrated</>
}
// /pages/+config.js

// Default value for all pages
export default {
  island: false
}

fwiw the {name}.page.{type}.ts felt intuitive to me

One thing I don't like about V1 is that it's not clear anymore where files are being loaded. The user has to know it. Someone reading VPS user-land code for the first time will have no clue. A workaroud is to implement IDE plugins that visually display where a file is being loaded. (Btw. contributions welcome to implement prototypes.) Also, I see it to be the framework's responsability to clearly communicate where files are being loaded. Frameworks that are well designed and simple, together with such IDE plugins, solves the problem, I believe.

the benefits

The foundational benefit here is that VPS knows everything about the user's app at build-time while enabling frameworks/users comprehensive control. The configDefinitions is the most powerful part of this RFC.

For the longest time it was believed that Next.js is the right level of abstraction. This RFC puts a big question mark on that assumption. Imagine a framework that deeply integrates with GraphQL.

// /pages/todo/+config.ts

import type { Config } from 'awesome-framework'

export default {
  query: '{ todo { text } }',
  Page: './TodoListPage.tsx',
  ssr: false // Not needed for a to-do list page
} satisfies Config
// /pages/todo/TodoListPage.tsx

// Ideally `awesome-framework` includes type generation
import type { Page } from './types'

export default function({ result }) {
  return <>{result[0].text}</> // displays "Buy milk"
} satisfies Page

Data fetching is completely taken care of by awesome-framework.

astahmer commented 1 year ago

makes sense ! thanks for the detailed explanations

AaronBeaudoin commented 1 year ago

I love this. 🤩

That doesn't mean I don't also have some questions/thoughts to add, however.


In your "islands" example above, how did node_modules/some-framework/+config.ts get loaded?

I suspect this question is a result of my lack of understanding, but how did VPS 1.0 find the +config.ts file at node_modules/some-framework/? I assume it doesn't search the entire node_modules directory for +config.js files, since that sounds like a performance nightmare. So then, how does it work? Am I correct in understanding that the package.json files look like this:

// node_modules/some-framework/package.json
{
  ...
  "version": "0.1.0",
  "dependencies": {
    "vite-plugin-ssr": "^1.0.0"
  }
}

// ./package.json
{
  ...
  "dependencies": {
    "some-framework": "^0.1.0"
  }
}

And not like this:

// node_modules/some-framework/package.json
{
  ...
  "version": "0.1.0"
}

// ./package.json
{
  ...
  "dependencies": {
    "vite-plugin-ssr": "^1.0.0", // Explicitly adding VPS as a dependency.
    "some-framework": "^0.1.0"
  }
}

Sorry if this is a bit of a dumb question!


I'd like to use my custom configDefinitions in my page files.

The configDefinitions idea is wild. I absolutely love it, because it lets me create a convenient "custom meta-framework API" for whatever I need for my project.

My question, however, is whether I can set my configDefinitions properties from my page files. In your "islands" example above (which blew my mind) you still set the island property in pages/about/+config.js. I'd like to just do export const island = true; from pages/about/Page.jsx. To explain why I would like to have this option, keep reading.


I'd also like to have a way to automatically have files be declared as a page.

The one "step backwards" in this new VPS architecture is the lack of any way for me to create some file pages/whatever/SomePage.jsx and have it automatically be picked up as a page without also having to add it in my pages/+config.js or pages/whatever/+config.js.

The thing is, although we now can have a "single" config file for VPS, it doesn't look like we can have a "set it once and never touch it again" config file. Every time I create a page, delete a page, modify a filename, or update anything about page's config (change it from server-only to client-only, or update a route) I need to head into my +config.js file. It is nice that this makes everything explicit, but it's also more time spent in configuration than I would like.

Maybe I could hack around this by having some custom file watching mechanism which updates my +config.js dynamically, but this seems like something I should be able to do a bit more easily, since VPS is already looking for some files automatically.

Here's my solution.

Give me a built-in pagesFilesystemPattern (or something like that) to tell VPS some set of files which should be interpreted as pages in the filesystem and automatically declared. It might look something like this:

{
  ...
  pagesFilesystemPattern: /.+Page\.jsx/g
}

In this case any files ending in Page.jsx are automatically discovered and declared as pages by VPS. If I want to further customize my pages config I can still create +config.js files anywhere in my pages/ subdirectories, in which case all pages at or below the current level will use the "overridden" config, but not require me to create pages properties in my +config.js files.

To have a bit more flexibility, I could also do something like this:

{
  ...
  pagesFilesystemPattern: {
    pattern: /(.+?)(Server|Client)?Page\.jsx/g,

    // Not if this would be the arguments exactly, but you get the idea.
    handler(path, match) {

      // Return value is an object to append to `pages`.
      return {
        Page: path + "/" + match[0],
        route: path + "/" + match[1].toLowerCase(),
        env: {
          "Server": "SERVER_ONLY",
          "Client": "CLIENT_ONLY"
        }[match[2]]
      };
    }
  }
}

I could even use this to create a simple but powerful compatibility layer to migrate an existing VPS project to 1.0, such that all my existing <whatever>.page.jsx files get picked up automatically.


The two ideas above combined result in the ability for me to define a +config.js file once and never touch it again. I can now give myself a way to create "filesystem" page files like is currently possible, but with my own custom convention, and then I can also give myself a way to affect the +config from inside my page file by doing some export const <whatever>.

I think this would be beautiful. I can basically have my own framework API from start to finish. If I've created a node_modules/some-framework/+config.js, then technically my own project can even have zero config. It also helps with the "what environment is this page running in" question, because by setting that environment from some custom property exported from the page itself I can see the environment from right inside the page.

brillout commented 1 year ago

@AaronBeaudoin

how did node_modules/some-framework/+config.ts get loaded?

By using VPS's Extensions API. There aren't any docs for it yet, but you can have a look at an example here.

Am I correct in understanding that the package.json files look like this

Yes, some-framework can hide the VPS dependency away from the user.

I'd like to just do export const island = true; from pages/about/Page.jsx.

That's not (easily) possible because configs (e.g. ssr or island) need to be known before transpilation begins, whereas Page.jsx can be loaded only after transpilation is done. The thing here is that +config.js is loaded before transpilation starts. (That's why it merely contains pointers to the actual application code.)

with my own custom convention

While I share the sentiment to give further control to the user, I'd rather go for "convention over configuration" for this one. At the end of the day, I don't see the added value of allowing users to configure this. But I'm open to change my mind if I can see concrete benefits.

The one "step backwards" in this new VPS architecture is the lack of any way for me to create some file pages/whatever/SomePage.jsx and have it automatically be picked up as a page without also having to add it in my pages/+config.js or pages/whatever/+config.js.

True.

I'm considering the possibility to create new pages in a succint manner:

/pages/index/+Page.js
/pages/about/+Page.js
/pages/jobs/+Page.js

Here, a new +config.js file isn't needed each time a new page is created.

But I'm currently leaning towards not supporting such succint syntax. But I may reconsider if users want this.

brillout commented 1 year ago

@AaronBeaudoin

I'm thinking of this:

// pages/+config.js

export default {
  // Tell VPS to also glob `+Page.js`
  glob: ['Page']
}
// pages/about/+Page.jsx

export default () => <>This page was created without creating/touching any +config.js file</>

I think it's a good trade-off.

npacucci commented 1 year ago

Hello everybody! I would like to give my feedback about this, and ask two questions.

I'm actually using VPS to build a custom framework based on islands architecture (as you have already mentioned), so every page is server-side rendered and partial hydrated client-side.

For me it's great to have a clear distinction between xx.server.tsx and xx.client.tsx code, in terms of DX and robustness (considering also dependencies, .css/.scss imports and informations that needs to be passed from server to client granularly).

Anyway, also describing this within a config file can be ok, and even more powerful for other scenarios.


First Question

First doubt is the same mentioned here. My cases are actually like:

test.page.server.tsx:

// pages/test/test.page.server.tsx

export { passToClient };
export { onBeforeRender };
export { render };

const passToClient = [ ... "stuff to serialize" ... ];
const onBeforeRender = () => { ... "init code" ... }
const render = (pageContext) => { ... "full HTML page construction" ... }

plus test.page.client.tsx:

// pages/test/test.page.client.tsx

export { render };

const render = (pageContext) => { ... "hydration code" ... }

And finally a custom route test.page.route.ts:

// pages/test/test.page.route.ts

export default '/test';

Reading your example I'm not sure if I got how this is gonna work with V1 purposed solution? Maybe like:

// pages/test/+config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  route: '/test',
  onBeforeRender: './test.page.before-render.server.ts', (Don't know if allowed name, should I put my "init code" here?)
  onRenderHtml: './test.page.render.server.tsx', // (Should I put my "full HTML page construction" code here?
  onRenderClient: './test.page.render.client.tsx', // (Should I put my  "hydration code" here?)
  ssr: true,
  Page: null, //(I don't need this, isn't it?)
  "stuff to serialize" here?? // (Should I list the actual passToClient strings?)
} satisfies Config

And even if I use a global +config, I assume what should I put inside it's the same as above, but for each page.


Second Question

Will be possible to read pages handlers as external packages from _nodemodules? Like:

// pages/test/+config.ts

import type { Config } from 'vite-plugin-ssr';

export default {
  route: '@test-page/route.ts',
  onBeforeRender:'@test-page/test.page.before-render.server.ts',
  onRenderHtml: '@test-page/test.page.render.server.ts',
  onRenderClient:'@test-page/test.page.render.client.ts',
  ssr: true,
} satisfies Config

Or even more, read the full page +config as external package and declare it somewhere else (maybe into the "Single Config File"?

brillout commented 1 year ago

@npacucci

// pages/test/+config.ts

import type { Config } from 'vite-plugin-ssr'

export default {
  route: '/test',
  onBeforeRender: './onBeforeRenderTest.ts', // "init code"
  onRenderHtml: './onRenderHtmlTest.ts', // "full HTML page construction"
  onRenderClient: './onRenderClientTest.ts', // "hydration code"
  ssr: true,
  Page: './Page.ts', // You *do* need this: you can only omit it for the root pages/+config.ts
  passToClient: ['pageProps']
} satisfies Config

After talking with the Munich team, it seems to me that the RFC is actually quite a good match with BurdaForward's use case. Feel free to PM on discord so we can elaborate. Also, FYI, a VPS sponsor is currently building a framework VPS + Solid + Partial Hydration (ideally as a re-usable rendered like https://github.com/brillout/stem-react / https://github.com/brillout/vite-plugin-ssr/tree/main/examples/stem-react).

Will be possible to read pages handlers as external packages from node_modules? Like:

Yes, that's actually a primary goal of VPS V1.

brillout commented 1 year ago

@npacucci

I forgot to reply about the following.

For me it's great to have a clear distinction between xx.server.tsx and xx.client.tsx code, in terms of DX and robustness

I agree, my previous comment about this:

One thing I don't like about V1 is that it's not clear anymore where files are being loaded. The user has to know it. Someone reading VPS user-land code for the first time will have no clue. A workaroud is to implement IDE plugins that visually display where a file is being loaded. (Btw. contributions welcome to implement prototypes.) Also, I see it to be the framework's responsability to clearly communicate where files are being loaded. Frameworks that are well designed and simple, together with such IDE plugins, solves the problem, I believe.

And even if I use a global +config, I assume what should I put inside it's the same as above, but for each page.

Correct.

Or even more, read the full page +config as external package and declare it somewhere else (maybe into the "Single Config File"?

I'm not entirely sure what you mean here, but you'll be able to publish npm packages that include +config.js files . (It's called the "Extensions API", you can see an example of the Extensions API with VPS 0.4 in the Stem React package I linked in my previous reply.)

npacucci commented 1 year ago

Great! I got it now.

My only doubt is still about this: Page: './Page.ts', // You *do* need this: you can only omit it for the root pages/+config.ts

Why this is needed? Cause even right now we are not using the Page feature, but only the Render() hook

My prev example about this:

// pages/test/test.page.server.tsx

export { passToClient }; export { onBeforeRender }; export { render };

const passToClient = [ ... "stuff to serialize" ... ]; const onBeforeRender = () => { ... "init code" ... } const render = (pageContext) => { ... "full HTML page construction" ... }

So I expect to only set onRenderHtml: './onRenderHtmlTest.ts', (That should be the same of the actual Render() hook, isn't it?).

Otherwise if I have to set both onRenderHtml and Page, what should I write inside this last?

brillout commented 1 year ago

@npacucci Got it. So, yes, you can omit Page if you define both onRenderHtml() and onRenderClient(). (These two hooks are functionally equivalent to the current two hooks .page.server.js#render and .page.client.js#render.)

brillout commented 1 year ago

Example of using the V1 design: https://github.com/brillout/vite-plugin-ssr/tree/dev/examples/v1.

Note that V1 is still work-in-progess and functionalities beyond this basic example isn't supported yet.

leonmondria commented 1 year ago

Nice, I like the fact that it's less magic (some might call it convention), by configuring it this way it's more predictable and readable.

I do think it's weird to see the + in the config file, never seen that before. Might scare some people, but might also just be something to get used to.

Small note, is there a reason why you are defining the imports with strings instead of direct imports?

import { onRenderHtml } from './config/onRenderHtml.tsx';
export default {
  onRenderHtml: onRenderHtml,
  // vs
  onRenderClient: './config/onRenderClient.tsx',
}
brillout commented 1 year ago

weird to see the +

Yea, I found it ugly at first but it grew on me and I really like it now. (FYI it's SvelteKit who introduced that idea.)

why [...] strings instead of direct imports?

Because the closure behavior would be confusing. Many criticize this about Next.js's getServerSideProps(), rightfully so.

// /pages/+config.js

let app;

export default {
  onRenderClient(pageContext) {
    app; // This `app` variable is another than below
  },
  onRenderHtml(pageContext) {
    app; // This `app` variable is another then above
  }
}
vchirikov commented 1 year ago

I don't see the clientRouting: boolean in the examples. Also I don't fully understand ssg/prerender section, where and how will it look like with parameters in routes?, For example will I be able to prerender only a part of values and use csr for all others?.

Could you provide the complete (for this moment at least) proposed typescript definition of the config? It may also be easier to discuss in a draft PR with commit of v1 typescript definition of config. (comments are better + could be resolved)

leonmondria commented 1 year ago

Because the closure behavior would be confusing. Many criticize this about Next.js's getServerSideProps(), rightfully so.

I agree, in that example it might lead to confusion. Still think it would be less magic/more predictable by moving the responsibility of actually importing, to the config file.

brillout commented 1 year ago

@vchirikov I will.

@leonmondria I share the sentiment here but I'm afraid directly importing in +config.js is quite problematic, for many reasons.

iMrDJAi commented 1 year ago

@brillout Here is a quick question: Is VPS going to continue supporting the current design?

Note: I find the + in +config.js too ugly, is the prefix gonna be customizable?

brillout commented 1 year ago

Is VPS going to continue supporting the current design?

The 0.4 branch will be supporting both for a little while and soft-deprecate the old design (show a warning instread of throwing an error). The V1 release will completely remove the old design.

Note: I find the + in +config.js too ugly, is the prefix gonna be customizable?

I found it ugly as well at first. But I grew to like it and I'm fairly confident you will as well. So no customization planned. As long as there are no functional regression then I think it's worth it to add a little bit of ugliness (in exchange for profound improvements, and also I've actually come to find it beautiful.)

samuelstroschein commented 1 year ago

Just here to express that we'd also like some glob matching of routes a la *.page.tsx.

https://github.com/brillout/vite-plugin-ssr/issues/578#issuecomment-1369874203 proposes the following syntax:

// pages/+config.js

export default {
  // Tell VPS to also glob `+Page.js`
  glob: ['Page']
}

I suggest making the glob explicit. There seems to be no benefit of implicitly matching js/ts files (what about .svelte, .vue, etc.). Furthermore, why would a + prefix be of benefit? Let users decide what to match explicitly:

export default {
  glob: ['*.page.{jsx|tsx}']
}
brillout commented 1 year ago

@samuelstroschein I'll think about it. (FYI there is a tension between the pro of having flexibility and the con of having too many options a la webpack. If they don't limit flexibiilty, then strong conventions, including naming conventions, are beneficial in the long term.)

brillout commented 1 year ago

I'm actually increasingly thinking of completely removing Filesystem Routing, including the aforementioned glob proposal. Single Route Files really are superior for anything non-trivial.

I understand that some folks want to build frameworks on top of VPS and want control over this. So I'm open to re-consider, but I'd like to engage into discussions: if you want Filesystem Routing then add a comment over there https://github.com/brillout/vite-plugin-ssr/discussions/630.

brillout commented 1 year ago

I've an idea how to make FS routing work very well with the V1 design. I'll show an example. We can consider the dicussion #630 as "closed" in the meantime.

harrytran998 commented 1 year ago

Hi @brillout, do you had decided on what the V1 design looks like(final decision) and the roadmap for this? Currently, I start to migrate our projects from Nextjs to VPS but seem this process might take a long time 😆.

brillout commented 1 year ago

Example of the new Filesystem Routing: examples/react-full-v1/. (E.g. see the + files in renderer/ and pages/hello/.)

@XiNiHa @AaronBeaudoin Ability to create custom configurations with deeper control than currently possible (the naming is messy but it will change and improve, I've already a couple of neat naming ideas): https://github.com/brillout/vite-plugin-ssr/blob/a9057be9beb08854034b5cc24572edf4292f1d02/examples/react-full-v1/renderer/+config.ts.

@harrytran998 The V1 is functionally equivalent to the current design (while allowing deeper control and customization than currently possible). So I recommend to start migrating to the current design and eventually migrate to the V1 design. The V1 design and the current design will co-exist for a while, so you'll be able to migrate on your own terms.

samuelstroschein commented 1 year ago

All the + files are too much boilerplate.

I created an alternative proposal https://github.com/brillout/vite-plugin-ssr/pull/674

redbar0n commented 1 year ago

For inspiration on simplicity and API, I would postpone finalizing the V1 design until you could glance at how the upcoming TanStack Start framework is designed. Tanner has an impeccable taste for making simple API's. https://tanstack.com/

brillout commented 1 year ago

@redbar0n What I like about what they're planning is typesafe routing and server code extraction (even though I disagree and I'd rather go with file based splitting as it's safer, see my reply in #674). But I don't think it's a reason for waiting on them. (FYI they don't have much yet – it's going to take a while until they ship something usable.)

redbar0n commented 1 year ago

I really don't like Page.js files, like in /pages/about/+Page.js, since they are confusing to discern from each other when multiple are open in IDE tabs (yes you can config IDE to show the path but most don't and it takes space).

Not sure if a + sign is even needed here, if vps could pick up on the file path or .page. in the filename as the convention. (I generally don't like + in filenames, aesthetically, and struggle to see why it's needed. Rails was able to do without it, based on convention, right?)

brillout commented 1 year ago

discern from each other when multiple are open in IDE tabs

Yes, it's an issue I do consider. (I've a couple of ideas in mind, let's see.)

Curious: do you know whether VS Code can be configured to show the path?

I generally don't like + in filenames, aesthetically

I guess it's a matter of taste. FYI I also didn't like it at first, but I grew to like it to the point that I now prefer it a lot more than .page..

why it's needed

The + prefix is about making it clear that it's is a VPS interface defining file. In general, I'd rather go for clear albeit more verbose than the opposite.

redbar0n commented 1 year ago

Nested routes can overwrite parent +config files.

VPS is configured via a root +config file with nested +config files per route.

I'd love this, I really would. But I'm not sure how/if we can achieve this.

I support the option to have a general / default config file, and an optional config file file near the components only if you need to override the default.

From https://github.com/brillout/vite-plugin-ssr/pull/674 :

That's why with the current 0.4 design configs can live in 5 different places:

.page.js
.page.client.js
.page.server.js
.page.route.js
vite.config.js (note that with the V1 design all configs previously defined in vite.config.js are now defined in +config.js)

I agree that config should live one place (for all pages), and optionally overridden at a single pre-determined other place (for each page). To not have to run around looking everywhere for the exact config.

Maybe the .page.route.js and vite.config.js for the page could be merged into a .page.config.js, since custom routing is a kind of config? It seems you've already thought this way, considering the headline "Single Config File (aka Single Route File)".

Alternatively, for exceptional cases, any .page.config.js could be subdivided into (and fully replaced by):

.page.client.config.js
.page.server.config.js

(This subdivision might be useful In case you want to make some distinction between the client side routing and server side routing, or have a Route Function run on the client but not the server. Route Functions being a kind of config.)

The folder could alternatively also act as the page denotation instead of having .page. on every file. Convention over configuration. So it would be /pages/error/error.config.js, and if you view only the filename, without the path, you would just need to know from convention that any component with a config is a page.

Instead of:

/pages/error-page/error.page.client.config.js

Or the suggested:

/pages/error-page/+config.js
# or even
/pages/error-page/+isErrorPage.js

One could have:

/pages/error/error.client.config.js

Maybe one could even drop the /pages/ directory (like Next.js 13 does):

/app/error/error.client.config.js

(One would treat any component with export { Page } as a page, unless you need a pagesFilesystemPattern.)

In case of no separate config between server/client, it would simply be:

/app/error/error.config.js

Doesn't that look nice?

(That config file would include any potential Route Function for the /app/error/error.js page.)

Then error.config.js would also be much more distinguishable in IDE tabs, than multiple +config.js files.


So for normal pages, instead of:

/pages/index/+Page.js
/pages/about/+Page.js
/pages/jobs/+Page.js

You'd have:

/app/index/index.js
/app/about/about.js
/app/jobs/jobs.js

Alternatively, if you need a page distinction to target with regex in a pagesFilesystemPattern, then this could work, and be close to the existing 0.4 format:

/app/index/index.page.js
/app/about/about.page.js
/app/jobs/jobs.page.js
redbar0n commented 1 year ago

Curious: do you know whether VS Code can be configured to show the path?

Yes. It can be configured. But most won't do it, tabs take more space (I think), and it still causes confusion from time to time.

I guess it's a matter of taste. FYI I also didn't like it at first, but I grew to like it to the point that I now prefer it a lot more than .page..

I see. First impressions matter, though, in the competition for users. If either is absolutely necessary, then I still prefer .page., since it is self-explanatory and requires no documentation/meta-language. I hate special symbols with hidden meanings. (Most people won't have used SvelteKit before.)

The + prefix is about making it clear that it's is a VPS interface defining file. In general, I'd rather go for clear albeit more verbose than the opposite.

Ok. I agree. Except where clarity can be obtained by convention over configuration, because otherwise accuracy can become overspecification.

redbar0n commented 1 year ago

This might be a bit radical for a V1, but just to throw the idea out there:

You could even take it one step further, removing the "page" metaphor and allow any component to have/modify the URL. Could be very powerful, and a unique selling point for a framework.


"The page metaphor is killing the web." https://twitter.com/alexbunardzic/status/1633147283490627590?s=20
 "The web is based on resources. Resources can be represented in countless ways. We chose to represent them as web pages. My hunch is that there's gotta be a better way to represent a resource on the web." https://twitter.com/alexbunardzic/status/1633161417607221248?s=20
 "Sharing resource representations is the use case. Pages are just one way resources on the web can be represented." https://twitter.com/alexbunardzic/status/1633253175393423361?s=20

Instead of making any component that export { Page } into a page, you could make any component that export { Url } affect the URL in some way.


Or even better, check out:


“That means we can avoid hard-coding href attributes and create reusable navigational components.“ … “Navigational components contain Hyperlinks and Hyperlinks aren't reusable because their href attributes tie them down to a particular UI. The Navigation router's data-first approach sets Hyperlinks free so you can reuse them across UIs.” https://grahammendick.github.io/navigation/documentation/reusable-components.html

redbar0n commented 1 year ago

https://github.com/brillout/vite-plugin-ssr/issues/341 Single Route File contains a bit of information on the design of routing, maybe relevant to the discussion here.

brillout commented 1 year ago

I agree that config should live one place (for all pages), and optionally overridden at a single pre-determined other place (for each page). To not have to run around looking everywhere for the exact config.

Yes and that's actually the case: everything is defined by /pages/+config.js (global) and /pages/some-page/+config.js (page-specific).

An expection being "shortcut" files such as:

Another exception is /renderer/+config.js which is equivalent to /pages/+config.js.

I understand the motivation for keeping VPS's interface minimal and I share that sentiment. But there are valid reasons to slightly bloat the interface with these two exceptions. (Although I'm thinking of removing /renderer/+config.js in favor of only supporting /pages/+config.js.)

Maybe the .page.route.js and vite.config.js for the page could be merged into a .page.config.js

That's already the case.

// /pages/movie/+config.js

export default {
  prerender: false,
  // This used to be defined in .page.route.js
  route: '/movie/@id'
}
// /pages/+config.js

export default {
  // This used to be defined in vite.config.js
  prerender: true
}

Btw. you can already play with the V1 design: it's live on the 0.4 release branch. Simply copy an exampe with a -v1 suffix.

run on the client but not the server

With V1 you configure where code lives in +config.js:

https://github.com/brillout/vite-plugin-ssr/blob/6ae376ef74527514afef1e61455679d68e140425/examples/react-full-v1/renderer/+config.ts#L6-L10

You can even create a custom config like this:

https://github.com/brillout/vite-plugin-ssr/blob/6ae376ef74527514afef1e61455679d68e140425/examples/react-full-v1/renderer/+config.ts#L11-L29

:warning: The naming will change and improve. (E.g. I'm thinking of renaming configDefinition to define although it would conflict with Vite's define so I'm not sure about the naming here.)

any component with a config is a page.

That's already the case.

/pages/error/error.config.js

To keep things simple I'd rather use a config: /pages/error/+config.js > export default { isErrorPage: true }.

Maybe one could even drop the /pages/ directory [...] /app/

I find pages/ 10x better/clearer than app/.

.config.js

Regarding .config.js VS +config.js.

I prefer this:

/pages/movie/+config.js
/pages/movie/+Page.js
/pages/movie/+onBeforeRender.js

over this:

/pages/movie/index.config.js
/pages/movie/index.Page.js
/pages/movie/index.onBeforeRender.js

That's the main reason why I prefer +config.js over .config.js.

As for IDEs, honestly, I'm inclined to simply tell users to configure their IDE accordingly. It's possible for VS Code which is, by far, the most used one IDE. (I don't know its JavaScript market share but I guess it's growing and getting close to like 80%?)

It can be configured. But most won't do it.

If someone is annoyed by it but isn't able to change his VS code config, then we really can't help him :-).

pagesFilesystemPattern

AFAICT it's a useless config. I'd rather find a good (enough) solution for everyone instead of giving the user too many useless options a la webpack.

I hate special symbols with hidden meanings.

I don't see much of a difference between /pages/movie/+config.js and /pages/movie/index.config.js. I'd actually say that the former is easier to guess.

"The web is based on resources. Resources can be represented in countless ways. We chose to represent them as web pages. My hunch is that there's gotta be a better way to represent a resource on the web."

In a future with digital interfaces la Minority Report, yes.

In the meantime, I think the page-centric web, as we know it today, works just fine. But I'm happy to be shown wrong with a concrete use case that adds enough added value to be worth exploring.

redbar0n commented 1 year ago

Maybe the .page.route.js and vite.config.js for the page could be merged into a .page.config.js. That's already the case.

Awesome. That's what I hoped. Thanks for the example.

any component with a config is a page. That's already the case.

Ok. But then why is a .page. or Page in the filename necessary to distinguish pages from other components?

I prefer this:

/pages/movie/+config.js
/pages/movie/+Page.js
/pages/movie/+onBeforeRender.js

over this:

/pages/movie/index.config.js
/pages/movie/index.Page.js
/pages/movie/index.onBeforeRender.js

That's the main reason why I prefer +config.js over .config.js.

I agree, in that case there. But how about?

/pages/movie/config.js
/pages/movie/page.js
/pages/movie/onBeforeRender.js

Because why do you need the + if you can rely on filename convention?

Alternatively:

/pages/movie/movie.config.js
/pages/movie/movie.page.js
/pages/movie/movie.onBeforeRender.js

The file with the same name as the folder would be treated as the index. (I hate index files, they're indistinguishable.)

(Capitalized P in Page seemed out of place, so I lowercased it instead.)

brillout commented 1 year ago

how about?

/pages/movie/config.js
/pages/movie/page.js
/pages/movie/onBeforeRender.js

The + is very much worth it, as it helps distinguish normal files from VPS files. It's asking too much from the user to remember all VPS's hooks and configs. Also, users (more realistically VPS frameworks) can create custom configs, so + is very much needed.

Alternatively:

/pages/movie/movie.config.js
/pages/movie/movie.page.js
/pages/movie/movie.onBeforeRender.js

That'd be an alternative indeed. But I like the current implementation better because it's more compact:

/pages/movie/+config.js
/pages/movie/+Page.js
/pages/movie/+onBeforeRender.js

What could convince me to change my mind is this:

As for IDEs, honestly, I'm inclined to simply tell users to configure their IDE accordingly. It's possible for VS Code which is, by far, the most used one IDE. (I don't know its JavaScript market share but I guess it's growing and getting close to like 80%?)

If someone can show me that enough IDEs with enough market shares cannot be configured, then I may reconsider the current design. Note that not many VPS users are complaining about this, so I'm inclined to think it isn't that big of deal.

P.S. the capital letter P in +Page.js should stay capital to keep things consistent. (It's exposed as pageContext.Page and not pageContext.page, which makes sense since it's a UI component and most UI frameworks have the naming convention to start UI components with a capital letter.)

harrytran998 commented 1 year ago

I agreed with brillout about the file name configs. Besides, I think the focus of V1 is not the file name, it can be updated in the future when a lot of users complain about that(I think it does not happen, like Svelte Kit).

If anyone comes to VPS, they care about the future, performance, and low-level architecture control. With the limited number of maintainers, I think DX should in loose mode.

brillout commented 1 year ago

it can be updated in the future

Indeed. Using "soft deprecations" (support the old interface but show a warning telling users to move to the new interface) we can introduce changes in any minor release. The 1.0.0 release is mostly a marketing thing (it will be equivalent to the last 0.4.x release minus the old interfaces). The V1 design is already released in 0.4.x.

That said, I'm more than eager to hear (critical) feedback.

I think DX should in loose mode.

I do care a lot about DX though :-).

x6ax6b commented 1 year ago

P.S. the capital letter P in +Page.js should stay capital to keep things consistent.

Is a lowercase p (+page.js) an error? It's tricky if no proper lint (ex: unicorn/filename-case) is provided. It's just a convention for now.

iMrDJAi commented 1 year ago

IMO if I have to worry about uppercase/lowercase in file names, then the new design won't be flexible at all.

brillout commented 1 year ago

@x6ax6b I believe https://github.com/brillout/vite-plugin-ssr/commit/334c303c15284ce539b486840895733683787179 addresses you concern.

@iMrDJAi Custom configs can have any name, built-in configs (should) have a unique name.

spacedawwwg commented 1 year ago

I really don't like the + at all. I agree with everybody else's sentiments on this.

Would 110% prefer @redbar0n suggested option over what feels like super, over opinionated file naming conventions.

/pages/movie/movie.config.js
/pages/movie/movie.page.js
/pages/movie/movie.onBeforeRender.js

If the + is a hill you're willing to die on @brillout (even though the feedback is pretty negative) can you not at least make the above an option for developers who want more verbosity?

"legibility trumps succinctness"

spacedawwwg commented 1 year ago

This is my current pages structure. This feel so much more organised and tidy. I'd hate to have to start implementing repetitive +Page.ts etc. I like being able to quickly scan my directory for file names.

image
x6ax6b commented 1 year ago

++1 It's a special file and should be at the top of the file list in all cases.

brillout commented 1 year ago

FYI SvelteKit also had quite some negative feedback on their + convention at first. But, now, it seems like no one is complaing about it anymore?

I really do understand it's weird, but I also believe it's the right decision. (Clarity is a lot more important than weirdness.) I'm happy to be shown wrong with some concrete arguments.

All-in-all, I wouldn't worry: worst case we can change things at anytime (by using the soft deprecation trick I explained in a comment above). Although, tbh, I'm pretty confident the same thing will happen than what happened with SvelteKit: users will eventually realize the maintainers were right all along. (FYI JSX was hated by a lot of people who ended up loving it later one.)

I do wonder though why you find it so disturbing to the point of writing such an emotional comment.

As for your example, I'd argue that this is better:

/pages/UcoHome/+Page.vue
/pages/UcoHome/+route.ts
/pages/UcoHome/+onBeforeRender.ts
/pages/UcoHome/types.ts

I actually prefer this a lot more. Not only by little, but considerably so.

In case you use VSCode, I'd suggest to configure VSCode to show the whole path instead of only the filename.