inertiajs / inertia-laravel

The Laravel adapter for Inertia.js.
https://inertiajs.com
MIT License
1.99k stars 222 forks source link

[2.x] Automatically disable wrapping for API resource props #619

Open TheDoctor0 opened 2 months ago

TheDoctor0 commented 2 months ago

Continuation of #432 as it remains a problem in the latest version of Inertia.

I was in the process of porting the project to Inertia and I encountered a very unexpected issue with the JSON resources serialization. Some had a different structure in comparison to the one returned from API routes - a data wrapper was present.

I tinkered with the source code and discovered that the issue lies in how resources are being serialized in the response master/src/Response.php#L136.

I was surprised to find out that the FakeResource used for tests has a $wrap property set to null to mitigate this issue but that has not been documented anywhere. Without this property, the test for response with JSON resource fails.

To fix this unexpected behavior I changed the logic for resource serialization to make sure not to use the data wrapper.

reinink commented 1 month ago

Hey thanks for this contribution!

This issue has come up in the past, see #93.

My concern with this change is that it's a pretty big breaking change that would require updating your page components to use the non-wrapped version of the data.

To this point I've recommended that people using resources put JsonResource::withoutWrapping in their app service provider, and that way folks can opt-in to this behavior by using this existing Laravel option.

We could bake this opinion into this adapter directly, but (at least from what I can tell) there would be no way to opt-out of it, so we need to make sure that's the right decision.

Unfortunately I don't use resources in my Inertia apps, so I can't really speak to this. I'm going to ask around to see what others think 👍

Either way it would have to be included as part of a major version release, which we are planning to do soon, so if we do want to make this change the timing is right.

LucaRed commented 1 month ago

I use JSON resources in my Inertia projects all the time. Why? So I can filter out data that I don't want to share publicly and add more properties at the same time (e.g. relations, computed attributes, etc...), all from a centralized location that I can reuse.

Personally, I've always found the data wrapping annoying, but I've never disabled it because of another annoyance: the $wrap property on JsonResources can be set to null to prevent wrapping, but it doesn't work when you instantiate a collection like this: ExampleResource::collection($models).

Related issues/PRs:

So, I'm all for disabling the JSON data wrapping for the next major release 👍

I understand that some people may not want to change all the references in their projects, so could we add a global configuration to revert back to the old data-wrapping behavior?

reinink commented 1 month ago

@TheDoctor0 Hey I think we're going to make this change for 2.x. Would you mind resolving the conflicts and also adding a test for this (assuming it's not too hard to test)?

TheDoctor0 commented 1 month ago

@reinink of course, I will update the PR whenever I have some free time this week.

reinink commented 1 month ago

@TheDoctor0 great, thanks!

TheDoctor0 commented 1 month ago

@reinink change ported to 2.x version and covered with tests.