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

adapter enhancement: revert exposing symbols #9683

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 12, 2024

Changes

  • We are planning refactors which will likely change where locals and client address are stored for the duration of rendering a page.
  • Currently they are stored as symbols on the request object.
  • Future features will likely involve multiple request objects per render, and the involvement of symbols in the new pipeline is uncertain.
  • Chances are, the current mechanism of passing data as symbol fields will become legacy, especially considering that the render() method of App now explicitly accepts options.
  • As such, we would not want to encourage the use of symbols.
  • This PR removes App.Symbol.locals, and App.Symbol.clientAddress from a feature branch not yet merged into main.
  • After this change, Adapter enhancements #9661 will be ready to review.

Testing

Existing tests should pass.

Docs

Removed the section for symbols from Astro 4.2 changest.

Copy link

changeset-bot bot commented Jan 12, 2024

⚠️ No Changeset found

Latest commit: 32dbe1e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Jan 12, 2024
@lilnasy lilnasy requested a review from ematipico January 12, 2024 17:27
@lilnasy lilnasy marked this pull request as ready for review January 12, 2024 17:27
@lilnasy lilnasy merged commit f7fa689 into withastro:adapter-enhancements Jan 13, 2024
13 checks passed
@lilnasy lilnasy deleted the app-symbols branch January 13, 2024 01:47
ematipico added a commit that referenced this pull request Jan 17, 2024
* quality of life updates for `App` (#9579)

* feat(app): writeResponse for node-based adapters

* add changeset

* Apply suggestions from code review

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* Apply suggestions from code review

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* add examples for NodeApp static methods

* unexpose createOutgoingHttpHeaders from public api

* move headers test to core

* clientAddress test

* cookies test

* destructure renderOptions right at the start

---------

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* Fallback node standalone to localhost (#9545)

* Fallback node standalone to localhost

* Update .changeset/tame-squids-film.md

* quality of life updates for the node adapter (#9582)

* descriptive names for files and functions

* update tests

* add changeset

* appease linter

* Apply suggestions from code review

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

* `server-entrypoint.js` -> `server.js`

* prevent crash on stream error (from PR 9533)

* Apply suggestions from code review

Co-authored-by: Luiz Ferraz <luiz@lferraz.com>

* `127.0.0.1` -> `localhost`

* add changeset for fryuni's fix

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

---------

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
Co-authored-by: Luiz Ferraz <luiz@lferraz.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* chore(vercel): delete request response conversion logic (#9583)

* refactor

* add changeset

* bump peer dependencies

* unexpose symbols (#9683)

* Update .changeset/tame-squids-film.md

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

---------

Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
Co-authored-by: Luiz Ferraz <luiz@lferraz.com>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
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) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants