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

Inertia::merge() Improperly Merges Pagination Objects in Inertia v2 #2068

Open
HichemTab-tech opened this issue Oct 26, 2024 · 15 comments
Open
Labels
react Related to the react adapter

Comments

@HichemTab-tech
Copy link

Version:

  • @inertiajs/react version: 2.0.0-beta.1

Describe the problem:

In Inertia v2, the new merge feature allows developers to merge new data into existing props instead of replacing them. However, there is an issue with how arrays that are nested within objects are being handled. Specifically, when an incoming prop is an object containing an array (e.g., pagination data), the array is not being merged properly.

Current Behavior:

  • In the source code:
    if (Array.isArray(incomingProp)) {
        pageResponse.props[prop] = [...((currentPage.get().props[prop] || []) as any[]), ...incomingProp]
    } else if (typeof incomingProp === 'object') {
        pageResponse.props[prop] = {
            ...((currentPage.get().props[prop] || []) as Record<string, any>),
            ...incomingProp,
        }
    }

You can check it here : source

  • If the incomingProp is an array, it is merged properly by appending the new elements to the existing ones.
  • If the incomingProp is an object, the properties are shallowly merged. This means that if the object contains an array (e.g., a pagination object with data, meta, and links), the array (data) is not merged correctly. Instead, it is replaced.

For example, consider a pagination object with the following structure:

{
    data: [/* items */],
    meta: { /* pagination metadata */ },
    links: { /* pagination links */ }
}

If you attempt to merge this object using Inertia::merge(), the data array will not be merged (appended to). Instead, it will be replaced by the new array, which leads to the loss of existing pagination items.

Expected Behavior:

  • When using Inertia::merge(), arrays inside objects (e.g., data within a pagination object) should also be merged, similar to how standalone arrays are merged.
  • Specifically, data within a pagination object should be appended to rather than replaced, while other properties (meta, links) should be replaced or handled appropriately.

Steps to Reproduce:

  1. Use Inertia::merge() in the backend to pass a pagination object as a prop.
  2. Observe how the data, meta, and links properties are merged in the frontend.
  3. Notice that while standalone arrays are merged correctly, the data array within the pagination object is replaced rather than appended.

Possible Solution:

  • Implement a deep merging strategy for nested arrays within objects. For example, if an object contains an array property, that array should be merged rather than replaced.
  • Alternatively, allow developers to define custom merge behaviors for nested properties to prevent unintended replacements.
@YounesAmalou
Copy link

Interesting observation, merge should actually merge the actual data object rather replacing it,

For example this might cause issues with paging if we try to send an object that contains the item list as an array + some meta data such as the max page, current page, next page...

Their might be other scenarios where the merge is actually needed.

This might not be convenient for people who expect to merge the actual data while not being familiar with this behavior,

I would suggest making this behaviors clear in the documentation while adding support for another method that handles the actual merge to keep both functions in case we actually need to replace the response.

@rugaard
Copy link

rugaard commented Dec 11, 2024

Seems to be the same issue or very similar in the Vue version as well.

@HichemTab-tech
Copy link
Author

HichemTab-tech commented Dec 11, 2024

Seems to be the same issue or very similar in the Vue version as well.

@rugaard yes seems to be related to the inertia core, well I suggested a solution for this in these PRs (inertia-laravel & inertia) where I introduced deepMerge, I hope they take a look at it soon.

@jaspertey
Copy link

I agree that this is a valid issue, and wanted to add my support for it. The examples shown in the docs: https://inertiajs.com/merging-props#server-side - would currently NOT work since they all return User::paginate() responses.

@MohammadZarifiyan
Copy link

Any updates?

@HichemTab-tech
Copy link
Author

Any updates?

Not yet unfortunately :"/ .

@dillingham
Copy link

maybe @joetannenbaum can save us

@HichemTab-tech
Copy link
Author

Hey @joetannenbaum , can you take a look at this please?

@tauseefsshah
Copy link

Hey @joetannenbaum can you please look into this. Infinite scroll and similar features that require merging pagination props aren't working 😅
This will be a big help, Thanks.

@joetannenbaum
Copy link
Contributor

Hi everyone. I will check into this soon. I'm parsing through a lot of issues and PRs right now, but this is indeed on my radar.

Just for some clarity: this was intentionally a shallow merge for now, with plans to enhance later. So it's currently working as expected, but I hear you that it needs to be altered to work with a deeper merge.

@dillingham
Copy link

Hi everyone. I will check into this soon. I'm parsing through a lot of issues and PRs right now, but this is indeed on my radar.

Just for some clarity: this was intentionally a shallow merge for now, with plans to enhance later. So it's currently working as expected, but I hear you that it needs to be altered to work with a deeper merge.

The v2 docs show merging laravel pagination which does not work..

@HichemTab-tech
Copy link
Author

HichemTab-tech commented Jan 11, 2025

Hi everyone. I will check into this soon. I'm parsing through a lot of issues and PRs right now, but this is indeed on my radar.

Just for some clarity: this was intentionally a shallow merge for now, with plans to enhance later. So it's currently working as expected, but I hear you that it needs to be altered to work with a deeper merge.

Thanks for the update, yes and I’ve already proposed a PR for deeper merge functionality and mentioned it in the issue. Would love for you to check it.

@HichemTab-tech
Copy link
Author

Hey @joetannenbaum ! any updates?

@MohammadZarifiyan
Copy link

Hey @joetannenbaum !
We are looking forward to this update.
Please let us know if there is a new update.

@MohammadZarifiyan
Copy link

This feature is not working correctly on @inertiajs/vue

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

No branches or pull requests

8 participants