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

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

TheDoctor0
Copy link

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

reinink commented May 27, 2024

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.

@reinink reinink changed the base branch from 1.x to 2.x May 27, 2024 12:52
@reinink reinink changed the title [1.x] Fix unexpected JSON resource serialization [2.x] Fix unexpected JSON resource serialization May 27, 2024
@LucaRed
Copy link

LucaRed commented May 27, 2024

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

reinink commented May 27, 2024

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

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

@reinink reinink changed the title [2.x] Fix unexpected JSON resource serialization [2.x] Automatically disable wrapping for API resource props May 28, 2024
@reinink
Copy link
Member

reinink commented May 28, 2024

@TheDoctor0 great, thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants