-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
base: 2.x
Are you sure you want to change the base?
[2.x] Automatically disable wrapping for API resource props #619
Conversation
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 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. |
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 Related issues/PRs:
So, I'm all for disabling the JSON 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 |
@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)? |
@reinink of course, I will update the PR whenever I have some free time this week. |
@TheDoctor0 great, thanks! |
a397e56
to
2db5eb2
Compare
@reinink change ported to 2.x version and covered with tests. |
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.