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

New handling for define:vars scripts and styles #3976

Merged
merged 13 commits into from
Jul 22, 2022
Merged

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Jul 19, 2022

Changes

  • Fixes define:vars handling for scripts and styles
  • Requires Fix define:vars compiler#470
  • For scripts with define:vars, we now wrap them in a unique scope with {} to avoid redeclaration errors
    • A previous version of this PR implemented new hoisting logic for script but this was deemed out of scope. We may revisit this in the future.
  • For styles with define:vars, we can now ignore them entirely. They are handled at the compiler level.

Testing

Tests updated

Docs

Bug fix only

@changeset-bot
Copy link

changeset-bot bot commented Jul 19, 2022

🦋 Changeset detected

Latest commit: c419857

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

This PR includes changesets to release 12 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution 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 Jul 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 19, 2022

⚖️ Bundle Size Check

Latest commit: 77b7e9c

File Old Size New Size Change
deserialize 0 B 382 B + 382 B

@natemoo-re natemoo-re self-assigned this Jul 20, 2022
This was referenced Jul 21, 2022
@natemoo-re natemoo-re marked this pull request as ready for review July 21, 2022 20:18
Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

lgtm, just needs a changeset :+1

Added one comment that shouldn't block a merge either way

packages/astro/src/runtime/server/index.ts Outdated Show resolved Hide resolved
@natemoo-re natemoo-re merged commit fbef6a7 into main Jul 22, 2022
@natemoo-re natemoo-re deleted the fix/define-vars branch July 22, 2022 15:14
@astrobot-houston astrobot-houston mentioned this pull request Jul 22, 2022
@janreges janreges mentioned this pull request Oct 25, 2022
1 task
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.

4 participants