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

V1 Design #578

Closed
brillout opened this issue Dec 27, 2022 · 105 comments
Closed

V1 Design #578

brillout opened this issue Dec 27, 2022 · 105 comments
Labels

Comments

@brillout
Copy link
Member

brillout commented Dec 27, 2022

The [email protected] 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
Copy link
Contributor

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

singleConfigFile contradicts the nested example?

  • Are nested +config files auto-imported?
  • Why is /pages/+config.ts a single file config when /pages/product/+config.ts is an additional config file? Are the examples related? If not, how are nested layouts achieved with a single config file?

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

@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
Copy link

makes sense ! thanks for the detailed explanations

@AaronBeaudoin
Copy link
Contributor

AaronBeaudoin commented Dec 28, 2022

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 brillout pinned this issue Dec 29, 2022
@brillout
Copy link
Member Author

@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
Copy link
Member Author

brillout commented Jan 3, 2023

@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
Copy link

npacucci commented Jan 3, 2023

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 node_modules? 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
Copy link
Member Author

brillout commented Jan 3, 2023

@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
Copy link
Member Author

brillout commented Jan 3, 2023

@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
Copy link

npacucci commented Jan 4, 2023

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
Copy link
Member Author

brillout commented Jan 4, 2023

@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
Copy link
Member Author

brillout commented Jan 9, 2023

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
Copy link

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Member Author

@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
Copy link

iMrDJAi commented Feb 5, 2023

@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
Copy link
Member Author

brillout commented Feb 5, 2023

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
Copy link
Contributor

samuelstroschein commented Feb 5, 2023

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

#578 (comment) 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
Copy link
Member Author

brillout commented Feb 5, 2023

@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
Copy link
Member Author

brillout commented Feb 5, 2023

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 #630.

@brillout
Copy link
Member Author

brillout commented Feb 9, 2023

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.

@brillout
Copy link
Member Author

@phiberber Seems like you're slowly appreciating the whole + thing after all :-).

Beyond redbar0n's point the issue with : is that it already denotes parameterized route segments in many other ecosystems.

Maybe - then, something cleaner than +

I like the fact that + can be interepreted as "adding something" which kinda makes sense. The - sign is weird from that perspective.

@phiberber
Copy link

I still hate it and it has been one of the three reasons I avoided VPS. But I'm seeing there's not that many options of symbols other than +, suffixes are bad, prefixes are good but they should not feel cluttered, which is my main problem with the + sign.

Seeing a large project tree with + signs is the same to me as looking to every ad at the Times Square, too much information to process.

Colocation feels badly designed to me still, it could maybe get better with proper IDE support.

+Page.ts
$Page.ts
@Page.ts

Page.ts
<Page.ts
_Page.ts
-Page.ts
.Page.ts
~Page.ts

To me - means more like an item of a list than subtraction or something negative, it's really clean and easier to read.

@brillout
Copy link
Member Author

That's actually something I'd be up for making a poll about and going with the result. But still premature.

Out of all options, I clearly prefer +. So I'd suggest we go for + and revisit the sign after a while. I'm hopeful you'll get used to it.

@samuelstroschein
Copy link
Contributor

What about vike.page.tsx, vike.config.ts to achieve grouping of vike related files and more explicitness than "+ means it's related to vike"?

@brillout
Copy link
Member Author

The + sign quickly ends up being as explicit as a vike. prefix. A vike. prefix is more explicit only for newcomers, and I don't think it justifies giving away the succinctness of the + sign.

I'd suggest we pause and resume the discussion after a couple of months after the old design is deprecated.

That said, I'm more than looking forward to hear about concrete issues, such as paper cuts around IDE. (I actually have an idea about it but I purposely don't write about it because, so far, I don't think it's enough of a priority for us to spend time on. Important to prioritize things correctly, otherwise Vike will never hit prime time.)

To sum up:

  • Let's go for the + sign and revisit after a couple of months.
  • If you stumble upon concrete issues, e.g. with your IDE, let us know about it.

Looking forward to ship the V1 (design).

@redbar0n
Copy link
Contributor

redbar0n commented Aug 28, 2023

@phiberber

I do also like - more than + purely aesthetically. But I think + may be better here.

To me - means more like an item of a list than subtraction or something negative, it's really clean and easier to read.

I agree, I never thought about it as subtraction and addition, in terms of file names. - is well established in file names, and should be intuitive to most.

But, related, an issue I have with YAML is that it’s confusing to quickly glance at something and tell whether it is a list or not. Because of - being present or not (esp. when nested).

That’s could actually be an issue here too, since it would look like some folder items in the IDE indicate they are part of a list, so the (inexperienced) user may be inclined to make all files in the folder be prefixed with - to be uniform. Or vice versa. Both may conflict with this library’s operation.

In all, I’m starting to come around to the + sign. Let’s just try it and see.

@brillout brillout mentioned this issue Sep 26, 2023
5 tasks
@Hebilicious
Copy link
Contributor

Hebilicious commented Sep 27, 2023

Everything looks exciting !

I have one minor suggestion :

export default {
  configDefinitions: [
	  {  name: 'island', handler(value) {}},
	  {  name: 'foo', handler(value) {}},
  ]
} satisfies Config

I would love to see a change to :

export default defineVikeConfig({
 extendConfig: {
   island: { handler: (value) => {} }, //object syntax
   foo : (value) => { } // function syntax
 }
})

The reasons why :

  • Using a config helper is helpful for people who don't want to use Typescript as it provides type info for them as well (you can supports satisfies too, SST does it iirc
  • extendConfig is arguably semantically more correct than configDefinitions
  • Using an object to define the config is shorter while remaining readable and extensible

@brillout
Copy link
Member Author

@Hebilicious It's actually already renamed, see https://vike.dev/meta. I find "meta" better than "extendConfig", given the fact that meta is also about modifying existing configurations.

As for the config helper, it didn't occur to me that it would help JS users. Neat & good to know. PR welcome if that's something you'd be up to.

I also like the idea of an object syntax. Maybe something like that:

// /pages/some-page/+config.h.js

import dataHook from './data.js'

export default defineConfig({
  // object syntax
  dataMeta: {
    value: dataHook,
    // By default `env` is 'server-only'
    env: 'server-and-client'
  },
  // direct syntax
  data: dataHook
})
// /pages/some-page/data.js

export default (pageContext) => {
  /* ... */
}

It's a succinct way to define both the config value and its options 👍.

@Hebilicious
Copy link
Contributor

@Hebilicious It's actually already renamed, see https://vike.dev/meta. I find "meta" better than "extendConfig", given the fact that meta is also about modifying existing configurations.

As for the config helper, it didn't occur to me that it would help JS users. Neat & good to know. PR welcome if that's something you'd be up to.

I also like the idea of an object syntax. Maybe something like that:

// /pages/some-page/+config.h.js

import dataHook from './data.js'

export default defineConfig({
  // object syntax
  dataMeta: {
    value: dataHook,
    // By default `env` is 'server-only'
    env: 'server-and-client'
  },
  // direct syntax
  data: dataHook
})
// /pages/some-page/data.js

export default (pageContext) => {
  /* ... */
}

It's a succinct way to define both the config value and its options 👍.

I'll open a PR then !

About meta, it's more likely that people associate it with metadata, metaverse or Facebook meta ...

In what context is meta about modifying configuration ?

In my opinion, the name should encapsulate something like define, extend, modify or add.

@brillout
Copy link
Member Author

I agree and I ain't 100% happy with the name "meta" but it's the best I could find so far. The use cases listed in https://vike.dev/meta are a good representation for what meta is used for. Contribution welcome to suggest a better name.

(Looking forward to your defineConfig() PR 👀)

@redbar0n
Copy link
Contributor

I find "meta" better than "extendConfig", given the fact that meta is also about modifying existing configurations.

how about metaConfig ?

@vchirikov
Copy link
Contributor

vchirikov commented Sep 29, 2023

@brillout it will be nice to have docs about v1 on vike.dev, because for now they're unlisted :\

@brillout
Copy link
Member Author

@redbar0n I think, given the context, metaConfig would be redundent.

// /pages/+config.js

export default {
  // Since we are in +config.js it's fairly clear that meta relates to "meta configuration".
  meta: {
    // ...
  }
}

@vchirikov I agree and it will happen as soon as the V1 design is out of beta. (And since we increasingly recommend users to use Bati and vike-{react,vue,solid}, which both use the V1 design, we should release the V1 design sooner rather than later.)

There is one little breaking change coming for the V1 design and then it should be ready for release. So it's coming soon.

@redbar0n
Copy link
Contributor

redbar0n commented Sep 29, 2023

@brillout yeah I thought about that, but then again, since a config can refer to anything then meta can refer to anything not necessarily a type of config. So with just meta then it might seem that you are configuring some particular thing called meta (re: all kinds of associations with metadata, metaverse or Facebook meta ...) and not generally applying a configuration of the configuration itself.. Anyways, for my part I found risking a little over-specification in this case wouldn't hurt. Better too clear than unclear.

@brillout
Copy link
Member Author

Technically, meta is a config, just like any other config. How a config is being used is completely up to the implementation that uses it (that's one of the beauty of the V1 design as it's very flexible/customizable). And, actually, meta isn't the only config that influences other configs, e.g. ssr: boolean changes the env of Page. (FYI this flexibility is paramount for vike-{react,vue,solid} and other upcoming integrations such as vike-relay.)

@Hebilicious
Copy link
Contributor

Hebilicious commented Sep 30, 2023

I agree and I ain't 100% happy with the name "meta" but it's the best I could find so far. The use cases listed in vike.dev/meta are a good representation for what meta is used for. Contribution welcome to suggest a better name.

(Looking forward to your defineConfig() PR 👀)

For alternative names :

  • extendMeta
  • extendDefinitions
  • extendConfig
  • extendKeys
  • extendMetaKeys
  • extendConfigKeys
  • extendConfigDefinitions

extend(...) can be replaced by add, additional, define, modify ...

Definitely agree with @redbar0n , meta by itself doesn't indicates whats happening whatsoever (definition was better in that regard). Semantically, extend can be used for both additions and modifications.

@redbar0n
Copy link
Contributor

Re: https://vike.dev/meta

My vote goes to customConfig since it's:

  • self-explaining and straightforward. Everyone has head about a "custom config" before.
  • not an overloaded term like meta. Not everyone has heard about a "meta config" before.
  • since it relates to the WHAT and not the HOW (create, update), it encapsulates both creation and update. It also makes sense since it is declaring a JSON object to send to Vike. Imperative names would make more sense for functions.
  • gives a good feeling that you can actually customize Vike to your hearts content.
  • implies that you'll rely on Vike's default config if you don't provide a custom one.

@brillout
Copy link
Member Author

brillout commented Oct 9, 2023

I actually did consider the name define, but I ended up going for meta as it does a better job at communicating that it's a config that influences configs, which is pretty much what "meta level" means. And define can mean everything really (it actually already means something for bundlers like Vite and webpack so, let alone for that reason, we cannot pick that name).

As for the other suggestion, they are misnomers as they convey either extending or modifying but not both. Same for custom (which I actually also did consider back then): it conveys creating new configs but it doesn't convey modifying existing configs. I understand that customConfig is slightly clearer but it's also clunky.

I think meta is good enough, so let me suggest we settle for it.

@redbar0n
Copy link
Contributor

redbar0n commented Oct 9, 2023

It’s intuitively understood by everyone that a custom config overrides a default config. It’s pretty much a standard.

Sometimes it’s the things we think about at first that are the best solutions, because that will also be what others intuitively think first. :-)

@phiberber
Copy link

Tailwind had that problem with theme replacing the default config instead of modifying it, they sure had a lot of questions because people weren't used to the extend prefix.

Not sure how much the extend prefix could be useful here, although it doesn't seem to me that it's really a config, the current state of the meta parameter looks to me more like route props than a config itself.

If I'm not wrong with the above thought, I'd say meta is better than extendConfig or customConfig, although in that sense I think props would be a better name.

@redbar0n
Copy link
Contributor

redbar0n commented Oct 19, 2023

Just saw this. It gives some precedence and inspiration:

https://remix.run/docs/en/main/file-conventions/remix-config#routes

remix.config.js

A function for defining custom routes, in addition to those already defined using the filesystem convention in app/routes. Both sets of routes will be merged.

@brillout
Copy link
Member Author

brillout commented Dec 4, 2023

The V1 design is out of beta and now the official design.

@brillout brillout closed this as completed Dec 4, 2023
@brillout brillout unpinned this issue Dec 4, 2023
@brillout
Copy link
Member Author

brillout commented Dec 6, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests