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

Revert "refactor: remove serialize-javascript (#3278)" #3304

Merged
merged 2 commits into from
May 5, 2022
Merged

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented May 5, 2022

This reverts commit 13c1f5f.

Changes

Testing

No

Docs

N/A, bug fix

@changeset-bot
Copy link

changeset-bot bot commented May 5, 2022

🦋 Changeset detected

Latest commit: 38f8fb7

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

This PR includes changesets to release 1 package
Name Type
astro Patch

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 May 5, 2022
@FredKSchott
Copy link
Member

Before we merge... do we want to support passing a custom class as a prop for the browser? I can't imagine how we could ever support that technically. Maybe we just call this intentionally not supported, and ask the user to serialize to a JS/JSON object instead.

I don't love that error message though, so if we went that route I'd turn this PR into something that creates a more clear error message with some helpful info on how to resolve.

@matthewp
Copy link
Contributor Author

matthewp commented May 5, 2022

@FredKSchott We need to define what we support, currently it's been "whatever serialize-javascript supports" and thus this regression. Rather than define it on the fly I think we need to take a step back and reassess the tradeoffs (The PR that removed individual script tags was reverted because of this issue) and make a decision on what to support, and then move forward with implementations.

@FredKSchott
Copy link
Member

Thoughts on using @nuxt/devalue instead of devalue, which seems to have added drop-in support for non-POJOs? https://www.npmjs.com/package/@nuxt/devalue

I'm not against reverting if we need to, but this feels small enough that we could that "what is expected behavior?" convo here now (or in Discord) vs. reverting and then having the convo.

This doesn't feel like something impacting many users, based on the bug description.

@matthewp
Copy link
Contributor Author

matthewp commented May 5, 2022

My concern is that we might want to break this again in the future to fix the "one script tag per island" problem and I'd rather come to one conclusion on what we support and break once than multiple times.

@FredKSchott
Copy link
Member

ah, gotcha. Okay, that makes sense to me. LGTM with either path.

@matthewp matthewp merged commit 3d901ca into main May 5, 2022
@matthewp matthewp deleted the revert-devalue branch May 5, 2022 19:56
@github-actions github-actions bot mentioned this pull request May 5, 2022
nonphoto pushed a commit to nonphoto/astro that referenced this pull request May 6, 2022
…astro#3304)

* Revert "refactor: remove serialize-javascript (withastro#3278)"

This reverts commit 13c1f5f.

* Adds a changeset
JuanM04 added a commit that referenced this pull request May 6, 2022
matthewp pushed a commit that referenced this pull request May 11, 2022
* Removed ignores

* Migration to v3

* More changes

* Remove legacy redirects

* Fail when there is no ENABLE_VC_BUILD

* Fix edge

* Updated readme

* Changeset

* Added static mode

* Updated documentation

* Updated shim

* Made edge work!

* Updated changeset

* Ensure empty dir

* Fixed redirects for dynamic paths

* Removed extra declaration

* Splited imports

* Updated readme

* Fixed some urls

* Deprecated shim!

* [test]: Vercel NFT

* Beautify

* Edge bundle to node 14.19

Vercel runs 14.19.1 (I've checked it manually)

* Re-added shim (#3304)

* Added `node:` prefix

* Use the same bundling as Deno for Edge

* Remove esbuild

* Fixed shim

* Moved nft

* Updated changeset

* Added note about Edge

* fix typo

* Added support for Node 16 (vercel/vercel#7772)
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
…astro#3304)

* Revert "refactor: remove serialize-javascript (withastro#3278)"

This reverts commit 13c1f5f.

* Adds a changeset
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.

🐛 BUG: "Cannot stringify arbitrary non-POJOs" at dev and build
2 participants