Allow passing initialData callback

inertiajs / inertia

Inertia.js lets you quickly build modern single-page React, Vue and Svelte apps using classic server-side routing and controllers.

https://inertiajs.com

MIT License

6.3k stars 423 forks source link

Allow passing initialData callback #1964

Closed RobertBoes closed 1 week ago

RobertBoes commented 4 weeks ago

solves #1960 and #1878 (although not through a new request)

This would allow users to customize how the initial data is rendered. Currently it is always added through the data-page attribute.

With this PR the following is possible;

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0" />

    <!-- Rendering the props in a script tag -->
    <script>
        window.__inertia_data__ = {{ Illuminate\Support\Js::from($page) }};
    </script>

    @vite('resources/js/app.js')
    @inertiaHead
  </head>
  <body>
    <!-- Just render a div, without the need for data-page -->
    <div id="app"></div>
  </body>
</html>

Then in your main script you'd do the following:

import { createApp, h } from 'vue'
import { createInertiaApp } from '@inertiajs/vue3'

createInertiaApp({
  resolve: name => {
    const pages = import.meta.glob('./Pages/**/*.vue', { eager: true })
    return pages[`./Pages/${name}.vue`]
  },
  // Pass the initialData callback, as we provide the data and Inertia doesn't have to grab it from the DOM
  initialData: () => window.__inertia_data__,
  setup({ el, App, props, plugin }) {
    createApp({ render: () => h(App, props) })
      .use(plugin)
      .mount(el)
  },
})

As a note: This would also require a bit of rework for SSR, as that would still output the div with the data-page. Haven't gotten to fixing that yet, but if there's no interest in this there's no point in researching that

pedroborges commented 3 weeks ago

I've reviewed the discussions around this topic, and I see the benefits of allowing more flexibility in how the initial data is rendered. I'd like to propose a slight adjustment to enhance the developer experience (DX) while maintaining backward compatibility:

Configurable Initial Data Rendering: Introduce a configuration option in the Laravel Inertia adapter that lets users choose between attribute or script for rendering initial data.
- If attribute is selected, the JSON-encoded props would continue to be rendered in the data-page attribute, as is currently done, using the @inertia directive.
- If script is selected, the props would instead be output as a JavaScript object in a <script> tag, leveraging the @inertiaHead directive.
Fallback Mechanism in createInertiaApp: Provide a default implementation of the initialData option in createInertiaApp that first looks for data in the <script> tag, and falls back to the data-page attribute if necessary. This keeps things flexible while allowing users to override the initialData option if they need a custom approach.

This would offer a cleaner and more flexible setup for users who prefer handling their data in a <script> tag while preserving the current behavior for those who prefer the data-page attribute.

RobertBoes commented 3 weeks ago

Not really sure I understand the proposal, you want to either load it from the data attribute or from a script (a predefined window variable?). That would be less flexible to the end-user, right? Or do you mean to pretty much keep this implementation, along with also checking a window variable. So the end user still has the option to overwrite the method and provide their own data?

jamesst20 commented 3 weeks ago

Just wondering, because we can already pass the page attribute to createInertiaApp as seen here https://github.com/inertiajs/inertia/blob/master/packages/svelte/src/lib/createInertiaApp.ts#L40

Wouldn't it be simpler to simply allow methods to be there?

page: string | () => string | () => Promise<string>

Replace

  const initialPage = page || JSON.parse(el?.dataset.page ?? '{}')

with

const initialPage = typeof page === "function" ? await page() : (page || JSON.parse(el?.dataset.page ?? '{}'))

usage

createInertiaApp({
  page: async () => {
    // Do whatever you want
    const response = await fetch("/api/initial-load")
    return await response.json();

   return JSON.parse(document.getElementById("script-id").textContent);
   return window.__initial_load__
  }
});

There wouldn't be any breaking changes. This wouldn't require any adapter update which is kind of tricky because there is not only the laravel one that needs to follow

Edit

The current proposal is likely to kill current SSR implementation because it currently expect a string to be returned.

pedroborges commented 3 weeks ago

@RobertBoes as @jamesst20 mentioned it seems that the current page argument in createInertiaApp can already address this use case. My proposal was aimed at improving the developer experience (DX) by not requiring the user to manually add the <script> tag. Instead, I suggest we add a inertia.pageSource option to the Laravel adapter and leverage the existing @inertiaHead directive to output the script tag when this option is set to script.

The only change inside createInertiaApp on the adapters would be:

- const initialPage = page || JSON.parse(el.dataset.page)
+ const initialPage = page || window.__inertiaPage || JSON.parse(el.dataset.page)

This way, the library supports the two most common use cases, script and data attribute, out-of-the-box and users would still have the flexibility to override page if they want to.

jamesst20 commented 3 weeks ago

@RobertBoes as @jamesst20 mentioned it seems that the current page argument in createInertiaApp can already address this use case. My proposal was aimed at improving the developer experience (DX) by not requiring the user to manually add the <script> tag. Instead, I suggest we add a inertia.pageSource option to the Laravel adapter and leverage the existing @inertiaHead directive to output the script tag when this option is set to script.

The only change inside createInertiaApp on the adapters would be:
- const initialPage = page || JSON.parse(el.dataset.page)
+ const initialPage = page || window.__inertiaPage || JSON.parse(el.dataset.page)
This way, the library supports the two most common use cases, script and data attribute, out-of-the-box and users would still have the flexibility to override page if they want to.

That would work but can we expect everyone https://inertiajs.com/community-adapters to update their implementation aswell? I do agree there is increased value into having it supported effortlessly in the backend adapter

I still believe supporting supporting a function/promise has its worth even if this could be done outside the createInertiaApp and passed as a prop

pedroborges commented 1 week ago

Hey @RobertBoes, I can confirm that this functionality is already supported via the page option in createInertiaApp, which allows you to pass the initial page data directly, like so:

createInertiaApp({
  page: window.initialPage,
  // ...
})

Does this approach satisfy your use case, or is there something additional you're looking to achieve with this PR? Let me know 😉

RobertBoes commented 1 week ago

Hey @pedroborges, yeah looks like that also works. I'll go with that then.