vike icon indicating copy to clipboard operation
vike copied to clipboard

New settings `inherit` and `isDefault` for controlling inheritance of cumulative configs

Open brillout opened this issue 1 year ago • 35 comments

Add two new settings isDefault: boolean and inherit: boolean for controlling inheritance of cumulative values:

// pages/+config.js

import { Layout } from './Layout'

export default {
  Layout: {
    value: Layout,
    // If a child defines a layout it will replace this layout
    isDefault: true
  }
}
// pages/admin-panel/+config.js

import { Layout } from './Layout'

export default {
  // Overrides all previously defined layouts (i.e. all layouts defined by ancestors)
  Layout: {
    value: Layout,
    inherit: false
  }
}
// pages/+config.js

import { Layout } from './Layout'

export default {
  // Completely opt-out of Layout inheritance: a +Layout always overrides all previously defined +Layout
  Layout: {
    inherit: false
  }
}

Groups

// pages/+config.js

import HeadDefaultOpenGraph from './HeadDefaultOpenGraph'
import HeadGlobal from './HeadGlobal'

export default {
  Head: [
    {
      value: HeadGlobal
    },
    {
      value: HeadDefaultOpenGraph,
      group: 'open-graph' // Can be any string (it's used as a unique identifying key)
    }
  ]
}
// pages/some-page/+config.js

import HeadOpenGraph from './HeadOpenGraph'

// This page inherits HeadGlobal but not HeadDefaultOpenGraph

export default {
  Head: {
    value: HeadOpenGraph,
    group: 'open-graph',
    // Alternatively we can add `isDefault: true` to the HeadDefaultOpenGraph value above
    inherit: false
  }
}

brillout avatar Jun 12 '24 10:06 brillout

I would like to be able to flexibly configure this inheritance. In my practice, a situation often happens when, for example, there are three pages: applications/listing applications/favorite applications/create

For the first two, you need to use a common complex Layout, for the latter, a simpler layout. Accordingly, I would like to be able to set my own Layout for /create, overwriting the original one.

However, there are also reverse situations when you need to specify the Layout for 1 of the many pages, for example: personalAccount/profile personalAccount/settings personalAccount/companies/portfolio personalAccount/companies/stats personalAccount/companies/about

In this case, I have added additional navigation for pages /companies - the ability to inherit previous Layouts is great

bighorik avatar Jun 19 '24 07:06 bighorik

@bighorik Yeah, currently the only way is to do this:

applications/(complex-layout)/listing
applications/(complex-layout)/favorite
applications/(simple-layout)/create

Let me know whether it's a showstopper for you and I'll bump the priority of this ticket.

brillout avatar Jun 20 '24 14:06 brillout

I'd love a way to "escape" from a parent layout defined in the renderer. In my app, I have

pages/{a bunch of pages}
pages/presentations/view
renderer/+Layout.jsx

pages/presentations/view needs to not have a surrounding layout, and currently I don't see a way to get it to not be present.

I have a workaround in renderer's layout, but I don't like it:

export function Layout({ children }: PropsWithChildren) {
  const pageContext = usePageContext();

  return pageContext.urlPathname.includes('presentations/view') ? (
    <MinimumPageShell pageContext={pageContext as any}>
      {children}
    </MinimumPageShell>
  ) : (
    <PageShell pageContext={pageContext as any}>{children}</PageShell>
  );
}

Moving the presentations/view page away from presentations/index and similar would feel weird to workaround the layout inheritance

AgentEnder avatar Jul 17 '24 20:07 AgentEnder

@AgentEnder Makes sense. I think this would be nice:

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

export default {
  // Remove all inherited layouts
  Layout: null
}

brillout avatar Jul 17 '24 20:07 brillout

That could work, currently I divide my PageShell component into MinimumPageShell and the base PageShell as saw above. Ideally I'd be able to set a Layout that is just the MinimumPageShell

AgentEnder avatar Jul 18 '24 02:07 AgentEnder

@bighorik Да, в настоящее время единственный способ - сделать это:

applications/(complex-layout)/listing
applications/(complex-layout)/favorite
applications/(simple-layout)/create

Дайте мне знать, подходит ли вам это для показа, и я увеличу приоритет этого запроса.

Unfortunately, this is not suitable. I have a layout common to all pages, which at some levels of nesting of the file system must be completely overwritten. I'm using vike version 0.4.171 in production, and the lack of backward compatibility makes me cry :(

bighorik avatar Oct 03 '24 19:10 bighorik

Unfortunately, this is not suitable. I have a layout common to all pages, which at some levels of nesting of the file system must be completely overwritten.

Can you elaborate?

I'm using vike version 0.4.171 in production, and the lack of backward compatibility makes me cry :(

The 0.4.x release branch is backwards compatible. If it isn't then I consider it a bug; elaborate if that's the case.

brillout avatar Oct 04 '24 09:10 brillout

@bighorik Yeah, currently the only way is to do this:

applications/(complex-layout)/listing
applications/(complex-layout)/favorite
applications/(simple-layout)/create

That seems a bit excessive if there's only one page that requires a specific layout, while all the other pages use the default layout.

alexmnv avatar Oct 14 '24 09:10 alexmnv

Labeling this as high-prio. I understand it's annoying, but I'd like to gauge how much and how often it's annoying in order to prioritize accordingly. The more I understand how annoying it is the more I will prioritize this.

brillout avatar Oct 14 '24 10:10 brillout

I have a layout with a sidebar and a right-hand panel that generally is centered with a maximum width, but occasionally needs to be broken out to take up the full right hand width. Currently, I would need to redefine the typical case on every page, and only occasionally leave out the layout so the child can take up the full width.

I bet there are other cases like this, but I wonder how the default and overrides should really work. There's definitely parts of the layout I want on every page, parts that I want on most pages, and parts that I want on some pages. There's a hierarchy there and I'm not sure whether it's defaults or overrides or some other way to configure the layout.

There are also cases where I want to swap out portions of the sidebar depending on the context, and not have that be dynamic, but just a part of those page's layout.

EDIT: This no longer matters to me. I set up a custom config flag that affects the layout. This works well for me.

chrisvander avatar Oct 14 '24 19:10 chrisvander

Indeed, using custom settings can be a workaround. Although note that all Layout code is loaded regardless of whether it's used then.

brillout avatar Oct 16 '24 06:10 brillout

Bumping to higest-prio (although it's the last prio among the highest prio).

I understand it's annoying, but I'd like to gauge how much and how often it's annoying in order to prioritize accordingly. The more I understand how annoying it is the more I will prioritize this.

Comment welcome. I could, maybe, implement this within a day.

brillout avatar Oct 21 '24 06:10 brillout

(Edit: After digging through the source code and having a cold shower, I think there's a better & generic solution, see next comment)

Just hit upon this myself, and was quite suprised that there isn't an out-of-the box solution for it. After thinking about this a bit, here's my two cents on it - the problem is a bit more complicated than it appears on first thought.

I'm going to use the following hypothetical structure as an example:

(blank root)
  (root folder)
    login/
    admin/
    article/
      ratings/
      details/
      seller-statistics/
    cart/
      checkout/
        delivery/
        payment/

Some observations about the problem space:

  1. Pages form a hierarchy with respect to their folder paths. The main goal of this hierarchy is to divide a larger problem domain into logical chunks that appear well-reasoned to developers, so using folders is a good match.

  2. Pages form a second hierarchy with respect to their URLs, which is, by default, derived from their folder path. This hierarchy is also mostly developer-oriented, so reusing the first hierarchy by default is a good match.

  3. Pages form a third hierarchy with respect to how their layouts are nested. This third hierarchy often aligns with the first hierachy, but not always, as it's determined not by developer-centric considerations but by "What is a logical UI nesting structure from a user's perspective?".

    There are four cases I can think of:

    1. Fully replacing all parent layouts (e.g. for a fullscreen login/ page).
    2. Inserting a page "further up the layout tree" (e.g. a ratings/ page that does not reuse the rest of the article layout, but only the rest of the page layout shell as defined at (root folder)).
    3. Moving a page to a different location in the layout tree (e.g. moving seller-statistics/ to be a layout child of admin/).
    4. Replacing a part of the parent layout (like the sidebar example provided by @chrisvander)

A few sidenotes:

  • Layout replacements should probably also apply to child pages (e.g. a checkout/ flow with a minimalistic layout that is also applied to delivery/ and payment/).
  • "Fully replacing all parent layouts" can also be modeled as "making the page a child of the (blank root)", i.e. the root folder but without any custom defaults.
  • "Replacing a part of the parent layout" can be modeled as the parent layout providing customization slots as well as defaults for that slot using the meta setting. It might make sense to add some sample code that shows how to do this to the documentation, but otherwise I think this problem is solved.

Based on these considerations, I would propose something like this:

// +config.ts
// Alternative 1:
export default {
  Layout: {
    parent: "/some/other/folder" | "../" | null,
    use: LayoutForThisPageAndChildren
  }
}

// Alternative 2:
export default {
  Layout: {
    extends: "/some/other/folder" | "../" | null,
    use: LayoutForThisPageAndChildren
  }
}

// Alternative 3:
export default {
  extendsLayout: "/some/other/folder" | "../" | null,
  Layout: LayoutForThisPageAndChildren
}

Based on alternative 2, here's a more complete example:

// src/pages/+config.ts
export default {
  Layout: DefaultLayout
}

// src/pages/login/+config.ts
export default {
  Layout: {
    extends: null, // Use (blank root) as parent
    use: LoginLayout
  }
}

// src/pages/article/seller-statistics/+Layout.ts
export default {
  extends: "/admin",
  use: function ({ children }) {
    ...
  },
}

// src/pages/cart/checkout/+Layout.ts
export default {
  extends: "/(minimal-layout)",
  use: CheckoutLayout
}

// src/pages/(minimal-layout)/+Layout.ts
export default {
  extends: null,
  use: MinimalLayout
}

cr7pt0gr4ph7 avatar Nov 07 '24 11:11 cr7pt0gr4ph7

After digging through the source code of vike and giving it some time, I think there's a better & more generic solution that can be achieved by introducing an inherits config setting that controls which properties (accumuluated or not) are inherited from parent folders:

// src/pages/+config.ts
export default {
  Layout: DefaultLayout
};

// src/pages/login/+config.ts
export default {
  inherits: {
    Layout: false,
  },
  Layout: LoginLayout
};

// src/pages/article/seller-statistics/+config.ts
const AdminDefaults = {
  inherits: {
    Layout: false,
    title: false
  },
  Layout: [AdminLayout, DefaultLayout]
};

export default {
  extends: AdminDefaults
};


// src/pages/cart/checkout/+config.ts
import MinimalLayoutDefaults from 'pages/(minimal-layout)/defaults';

export default {
  extends: MinimalLayoutDefaults,
  Layout: CheckoutLayout
};

// src/pages/(minimal-layout)/+config.ts
export default {
  inherits: {
    Layout: false,
  },
  Layout: MinimalLayout
};

// src/pages/(minimal-layout)/defaults.ts
export default const MinimalLayoutDefaults = {
  inherits: {
    Layout: false
  },
  Layout: MinimalLayout
};

cr7pt0gr4ph7 avatar Nov 07 '24 12:11 cr7pt0gr4ph7

I am also interested in this inherited layout behavior. One thing that really caused some interesting behaviors is that my _errors/+Page.js file is also inheriting the default layout, which it shouldn't be. I had to basically move all of my base routes into a (app) folder to prevent errors from using the default layout.

I get the entire Vike stack in a bit of a recursive trace because if something errors out in the default layout, it calls the error page, which calls the default layout with an error again.

+1 for the inherits example above. I read over the initial suggestion and I'm not sure I quite understand the expected behavior with default/overrides option. It seems like it covers the needed scenarios, but it's not all that clear. I probably just need to mull it over.

drew-dulgar avatar Nov 08 '24 00:11 drew-dulgar

I really need this option. In my app, some subpages won't inherit the layout from the parent. I can't implement this gracefully at the moment.

k9982874 avatar Nov 17 '24 02:11 k9982874

@k9982874 For now, you can use a simple workaround: just define your own config setting for Layout rendering, which will not be cumulative. Let's call it PageLayout:

meta: {
	PageLayout: {
		env: {server: true, client: true},
		cumulative: false,
	},
}

You can then render PageLayout inside the base layout. Here's an example using Vue:

<script setup lang="ts">
import {usePageContext} from 'vike-vue/usePageContext';

const pageContext = usePageContext();
const PageLayout = pageContext.config.PageLayout;
</script>

<template>
	<PageLayout>
		<slot />
	</PageLayout>
</template>

This approach allows you to define PageLayout separately for each page (without cumulation)

alexmnv avatar Nov 17 '24 06:11 alexmnv

@brillout After thinking about it a bit more especially with respect to composability, and taking @alexmnv's suggestions as well as the applicability to other settings like head into account, I now see the value of your original proposal.

When I write { myConfigSetting: ... }, my intent is to do one of four things:

  • Overwrite any inherited value completely
  • Append to array of inherited values
  • Replace a specific earlier element in array of inherited values
  • Merge new value with inherited value in some custom fashion
    • This includes merging objects (think of Object.assign), because one somehow has to specify how sub-properties should be merged.

Values at higher hierarchy levels usually have a default intent, i.e. what should happen when a child page specifies the same setting:

Array-like
(e.g. Head, Layout)
Array-like default
(e.g. Layout
sometimes?)
Object-like
(e.g. meta)
Primitive-like
(e.g. title)
Overwrite :heavy_check_mark: :heavy_check_mark: :heavy_check_mark: :white_check_mark:
Append :white_check_mark: :heavy_check_mark: :x: (makes no sense) :x: (makes no sense)
Replace :heavy_check_mark: :white_check_mark: (replace this,
keep inherited)
:heavy_check_mark: (Same as Overwrite)[^2]
Merge :heavy_check_mark: (but not useful)[^3] :heavy_check_mark: (but not useful)[^3] :white_check_mark:[^4] :heavy_check_mark:[^5]
  • :white_check_mark: Default intent
  • :heavy_check_mark: Possible alternative intent

[^2]: Replace is the same as Overwrite for primitive values, because there is only ever one inherited value available.

[^3]: Overwrite, Append and Replace already cover all useful behaviors for Array-like settings. Alternative stuff like also reordering inherited values is possible, but probably does not make sense from a readability/composability standpoint.

[^4]: Default merge behavior for properties of Object-like settings has to be specified recursively.

[^5]: Although this can also be implemented as separate config settings plus a computed setting.

cr7pt0gr4ph7 avatar Nov 19 '24 14:11 cr7pt0gr4ph7

Previous version of this post:

~It could be useful to have a per-setting customizable merge hook (like ConfigEffect, specified via meta, invoked at build-time) to perform the merge/post-process the merged value that will be seen by the consumers of the configuration setting. Two signatures are possible:~

  • ~merge(inheritedValue?: T, newValue: T): T~
  • ~mergeAll(inheritedValues: T[]): T~

~The first option is IMO more "functional" & from a technical standpoint, and ensures some sort of consistency when merging, and might be more amenable to tree-shaking. The second option is probably a better fit for the mutable JS data structures, though.~

~It could also be very useful to have a postProcess(mergedValue?: T): T hook that receives the merged value and can do some context-specific post-processing, inclulding materializing the config value from thin air.~

Well, it seems that this is already implemented for internal use as a _computed hook for some internal settings. +1 for making/documenting it as a public interface, because it already allows implementing all of the merge behaviors mentioned above.

cr7pt0gr4ph7 avatar Nov 19 '24 14:11 cr7pt0gr4ph7

I've rewrote my proposed design. It should be clearer now. (Indeed, naming the setting inherit is a lot more clearer :+1:)

As for merging values the idea is that merge behavior must be defined by the setting consumer. (This is what Vike does internally.)

Let me know if you believe my proposed design is still lacking. I suggest we proceed like this: can you find a use case where my proposed design doesn't work? This will probably lead a fruitful discussion.

Looking forward to settle on a design. It's indeed an essential feature that we should implement soon. (Maybe even this week if we make progress on the design discussion, but let's see.)

brillout avatar Nov 25 '24 20:11 brillout

I'm curious how essential this is. I'm finding that using custom settings works great for the more complex use cases. What parts would be best to leave to the individual implementation? I assume this comes back to the amount of component-level logic we're willing to tolerate.

With that said the ability to "punch out" with an inherit flag is a great idea, as is using a group to link with a specific ancestral layout.

chrisvander avatar Nov 30 '24 15:11 chrisvander

Indeed, the proposed design can already be used today by reproducing it with a custom setting. It's just about improving the DX. It isn't about unblocking users. That said, it's a very a recurrent question so having it built-in would be nice.

For edge cases, it's probably better to teach users instead of having something built-in. (Actually, I'm realizing maybe we should teach how to implementing grouping instead of having it built-in. I guess we can skip grouping for now and making it built-in later if there is frequent demand for it.)

brillout avatar Nov 30 '24 16:11 brillout

To me, it seems that the most pressing need may be addressed by just the inherit punch-out option. That would be a smaller feature that could be expanded on if there's a better DX. If someone wants to do a more sophisticated layout, they could compose by punching out and then importing the appropriate components.

chrisvander avatar Nov 30 '24 16:11 chrisvander

Good point. Let's for now only implement inherit: boolean + teach about advanced use cases.

brillout avatar Nov 30 '24 16:11 brillout

Looking forward to settle on a design. It's indeed an essential feature that we should implement soon.

I guess the design is settled: I'll implement a new inherit option.

// pages/admin-panel/+config.js

import { Layout } from './Layout'

export default {
  // Overrides all previously defined layouts (all layouts defined by an ancestor configuration)
  Layout: {
    value: Layout,
    inherit: false
  }
}
// pages/+config.js

import { Layout } from './Layout'

export default {
  // Completely opt-out of Layout inheritance: a +Layout always overrides all previously defined +Layout
  Layout: {
    inherit: false
  }
}

Let me know if this doesn't work for your use case.

brillout avatar Dec 21 '24 12:12 brillout

I'm considering implementing this, but I've one open question left. It would be nice to have +Layout.inherit.js similar to +data.{server,client,shared}.js because, otherwise, it's a bit of a pain to have to create a +config.js file just to define inherit: false.

But we need the user to opt-out of inheritance, so I'm thinking one of the following options:

  • +Layout.noInherit.js
  • +Layout.!inherit.js <= would be my favorite but unfortunately doesn't seem to play nice with Linux
  • +Layout.override.js <= renaming inherit: false to override: true

brillout avatar May 01 '25 18:05 brillout

Or maybe +Layout.-inherit.js. This should work on all filesystems. Con: looks a bit ugly, pro: it's clear (once you know what it means). Hm, not sure.

brillout avatar May 01 '25 18:05 brillout

Alternatives:

  • +Layout.~inherit.js
  • +Layout.^inherit.js

brillout avatar May 01 '25 18:05 brillout

When talking only about property names, I prefer inherit: false over override: true, as the former in my opinion makes it more clear what is being overridden.

+1 for +Layout.override.js, as that name probably makes the most sense even when one doesn't yet know about the inheritance mechanics. Every other name involving some symbol like ~, ^, ! is unclear without consulting the documentation, so .override.js seems like the least worst option.

...In conclusion, although having .override.js translate into inherit: false is not immediately obvious, at least the two variants by themselves are self-teaching, and can be understood as-is when they appear in the code.

cr7pt0gr4ph7 avatar May 07 '25 13:05 cr7pt0gr4ph7

@cr7pt0gr4ph7 How about +Layout.noInherit.js?

So far I'm hesitating between +Layout.noInherit.js and +Layout.-inherit.js. If I imagine myself using +Layout.-inherit.js regularly then I think I would get used to it and it would fit nicely, but it does look a bit cryptic 😅

brillout avatar May 07 '25 13:05 brillout