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

Generate static typename for element metadata key value pairs #876

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

cornedor
Copy link
Contributor

@cornedor cornedor commented Aug 28, 2024

Element metadata key/value pairs generate different __typename's on each request. This behavior makes it very hard if you need to work with the type.
This behavior also causes issues with Apollo Router, causing it to return null instead of the value since the typename does not match the loaded super graph.

The response is always the same type, an object with two strings, name and value, so there is no need to generate separate type names on each request.

Copy link

github-actions bot commented Aug 28, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Copy link

@cornedor
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@mattamon
Copy link
Contributor

mattamon commented Sep 4, 2024

@fashxp do you have any idea why it is unique?

@fashxp
Copy link
Member

fashxp commented Sep 19, 2024

@mattamon no, I have no idea.

@mattamon
Copy link
Contributor

mattamon commented Oct 2, 2024

@fashxp okay thanks! So basically this would also be a breaking change no?
If someone somehow managed to deal with the unique keys, this would break the system if now it is the same.

@cornedor
Copy link
Contributor Author

cornedor commented Oct 2, 2024

@mattamon Personally, I would not categorize this as a breaking change.

__typename is a type name, and should be static. The current behavior results in a breaking change for every request and does not follow the specs, since it is not possible to be used to determine Union types or cache keys.
This is the same as relying on a bug causing a 500 error, but that would not mean fixing it would result in a breaking change.

@fashxp
Copy link
Member

fashxp commented Oct 2, 2024

I would agree with @cornedor.
Cannot imagine a constellation where someone could rely on that, can you?

@mattamon
Copy link
Contributor

mattamon commented Oct 2, 2024

I would agree with @cornedor. Cannot imagine a constellation where someone could rely on that, can you?

I cannot, but there are some very creative solutions out there. :)
Then we should merge it. :)

@mattamon mattamon added this to the 1.9.0 milestone Oct 2, 2024
Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for clearing this up!

@mattamon mattamon merged commit e55bd44 into pimcore:1.x Oct 2, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants