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

Remove nested JSON.stringify from props serialization #7995

Merged
merged 2 commits into from Aug 9, 2023

Conversation

belluzj
Copy link
Contributor

@belluzj belluzj commented Aug 8, 2023

Changes

  • Only call JSON.stringify once when serializing props for an island
  • Match this change when deserializing props for an island
  • This fixes a quadratic increase in quote escaping when using nested data, see Exponential bloat in prop serialization #7978

Testing

I added a test that the kind of nested data from #7978 results in a normal-sized serialized text.

I didn't add more tests in this branch because I'm not comfortable changing the code organization in the project but I committed to another branch how I tested the implementation, basically by adding roundtrip tests to the file serialize.test.js. However I had to move the deserialization to the same file as the serialization, which I'm not sure fits the project structure. See daltonmaag@a513407

Docs

I believe this change should be transparent to users so I'm not sure it needs documenting?

@changeset-bot
Copy link

changeset-bot bot commented Aug 8, 2023

🦋 Changeset detected

Latest commit: 5b9a603

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 8, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

This looks good to me as a hotfix for astro@2.x. We'll want to revisit this approach for astro@3.x to avoid the &quote; encoding issue entirely.

@ematipico ematipico merged commit 79376f8 into withastro:main Aug 9, 2023
13 checks passed
This was referenced Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants