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

Use devalue for island props serialization #12093

Closed
wants to merge 4 commits into from
Closed

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 30, 2024

Changes

fixes #12088
partially revives #8004

compared to #8004, this uses devalue instead of seroval as I don't really understand the main difference with seroval, and we're already using devalue in our codebase. It also doesn't completely serialize other island information, only the props, to minimize the change surface area.

Testing

added test for #12088 and removed some irrelevant ones

Using the repro in #7978, this reduces the serialized prop value size ("Astro bytes"):

Before (4.15.9) After (this PR, v5)
Screenshot 2024-09-30 at 10 13 58 AM Screenshot 2024-09-30 at 10 15 17 AM

It could get even lower by serializing string values with single quotes to prevent needing escaping, but I think for now it's fine as a stop gap.

Docs

It doesn't look like we document specific serialization edge cases other than https://docs.astro.build/en/guides/framework-components/#passing-props-to-framework-components, which is still true today (regarding function serialization)

Copy link

changeset-bot bot commented Sep 30, 2024

🦋 Changeset detected

Latest commit: 1759606

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 Sep 30, 2024
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Sep 30, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

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 great, thanks for reviving my efforts! 🎉

For context, seroval is used by Solid and has more comprehensive support for common cross-environment primitives. The other benefit is that we wouldn't need to generate the (0, eval)() step, because seroval outputs self-contained JS strings.

Of course, devalue is also a great choice—I'll leave the final call up to you.

@bluwy
Copy link
Member Author

bluwy commented Oct 1, 2024

For context, seroval is used by Solid and has more comprehensive support for common cross-environment primitives.

I did some checks and I think they're largely the same. devalue can handle typed arrays, holey arrays, and other edge cases well. Except Error objects but I think that's fine and not something users usually do.

The other benefit is that we wouldn't need to generate the (0, eval)() step, because seroval outputs self-contained JS strings.

I think both devalue and seroval outputs similar JS that still requires us to eval. We're putting the values in attribute values, so we have to manually eval it again.

devalue also seem to be much smaller, which reduces the ssr bundle size. Comparison: devalue (5.21 kB -> 2.16 kB (gzip)) and seroval (28.2 kB -> 7.75 kB (gzip))

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

This is great. Makes a lot of sense that this matches the supported types for content layer too.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

I don't think we should introduce eval. Currently users are required to use unsafe-inline if they are using a CSP policy, this change would require that they also use unsafe-eval. Given the number of people who are not satisfied with an unsafe- at all, we should be moving in the direction of not requiring it, and I don't see how we can make CSP work with eval in the code.

@ascorbic
Copy link
Contributor

ascorbic commented Oct 1, 2024

This could use devalue.parse and devalue.stringify instead of eval.

@matthewp
Copy link
Contributor

matthewp commented Oct 1, 2024

@ascorbic how much client JS are we adding by bringing devalue in? The reason we went with a custom thing is because the file sizes of these tools seemed like too much. Also remember that it gets inlined into the page, you really don't want a several kilobytes dependency to be inlined like that, it's a waste.

Also consider here that the only thing we gain by bringing in a dependency is not needing to maintain our custom serializer/deserializer. But we haven't had much maintenance on the one we have. This Infinity thing is the last time we've gotten a request for a new type in probably a couple of years. Adding more kB to everyone's pages to avoid occasionally updating a small amount of custom code doesn't seem like a very good tradeoff to me.

@natemoo-re did a spike a while back (last year IIRC) on going back to inlining the hydration script rather than using astro-island. If we did that we could use these sorts of tools again because there would be no client-side penalty for deserialization. I think that's probably what we should do instead of this. And for now, just add Infinity as a new type in our custom serializer.

@natemoo-re
Copy link
Member

@matthewp supporting Infinity in the custom logic seems like a reasonable conclusion to me! NWTWWHB.

As Astro pushes into more ecomm/interactive usecases, the potential payoff on serialization optimizations will continue to grow. Could pair nicely with any cross-island communication explorations, if that's ever on the table.

@bluwy
Copy link
Member Author

bluwy commented Oct 2, 2024

going back to inlining the hydration script rather than using astro-island. If we did that we could use these sorts of tools again because there would be no client-side penalty for deserialization. I think that's probably what we should do instead of this.

We can't do that because that will require unsafe-inline again.

how much client JS are we adding by bringing devalue in?

devalue stringify is 971B gzip. Our custom implementation is 283B gzip.


Looks like there's no other way to implement this without size tradeoffs. I do still think the edgecases being handled by devalue is valuable but maybe in the future.

@bluwy bluwy closed this Oct 2, 2024
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) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants