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

fix: serialization warning when primitives have prototype changed #2902

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

SoaresMG
Copy link

πŸ”— Linked issue

No issue, do I need to open it?

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Unfortunately there are a few libs that inject utils into prototypes, with the current implementation of checkSerializableRuntimeConfig if String or any other primitive has an util injected into the prototype a warning is thrown.

I don't see any direct consequence of just ignoring it during the check, if that prototype is indeed needed it will be there once JSON.parse is used.

An example of this is forEachEmbeddedItem where the ContentStack team added an util into String.prototype.

I don't think libs should be polluting prototypes needlessly when it could very well be an exported function, but nitro shouldn't also be concerned about it since there is no consequence at all (right?).

Reproduction

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@SoaresMG SoaresMG requested a review from pi0 as a code owner November 25, 2024 11:42
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.

1 participant