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

fix: Reflect the configuration for esbuild #12676

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

koyopro
Copy link
Contributor

@koyopro koyopro commented Dec 7, 2024

Changes

I modified compileAstro to reflect the settings in viteConfig.esbuild.

This change ensures that when vite.esbuild.target is specified in the Astro config, it is reflected during the compilation of Astro files. Previously, while the vite.build.target value would affect the build command output of an Astro project, it did not impact the dev command output. As a result, the esbuild target was effectively fixed to esnext. This update addresses and resolves that issue.

For example, by setting the configuration as follows, you can specify the esbuild target during the compilation of Astro files.

// astro.config.mjs
export default {
  vite: {
    esbuild: {
      target: 'es2018',
    },
  },
};

Fixes: The vite.build.target setting does not take effect in dev mode. · Issue #12655

Testing

Added tests with code that includes syntax targeted by esbuild, confirming the following two points:

  • No transformation occurs if esbuild.target is not present in ViteConfig.
  • Transformation occurs if esbuild.target is specified as anything other than esnext in ViteConfig.
    • Tested with esbuild.target set to es2018.

Docs

No documentation changes are necessary.

Copy link

changeset-bot bot commented Dec 7, 2024

🦋 Changeset detected

Latest commit: 3d58791

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 Dec 7, 2024
Comment on lines +40 to -41
...compileProps.viteConfig.esbuild,
loader: 'ts',
target: 'esnext',
Copy link
Contributor Author

@koyopro koyopro Dec 7, 2024

Choose a reason for hiding this comment

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

Since the default target in esbuild is esnext, the explicit specification has been removed.
(If the user does not specify vite.esbuild in the astro config, compileProps.viteConfig.esbuild will be undefined, and the default settings of esbuild will be applied)
https://esbuild.github.io/api/#target:~:text=The%20default%20target%20is%20esnext

Copy link

codspeed-hq bot commented Dec 7, 2024

CodSpeed Performance Report

Merging #12676 will not alter performance

Comparing koyopro:feature/esbuild-target (3d58791) with main (e21c7e6)

Summary

✅ 4 untouched benchmarks

@koyopro koyopro marked this pull request as ready for review December 7, 2024 15:01
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Works for me 👍 I don't know if we can always guarantee that the Astro TS will always be affected by vite.esbuild, since TypeScript compilation within Astro is an implementation detail, but since we had use the API for long now I think this should be fine.

Comment on lines 13 to +14
async function compile(source, id) {
const viteConfig = await resolveConfig({ configFile: false, ...inlineConfig }, 'serve');
Copy link
Member

Choose a reason for hiding this comment

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

Can we pass the inlineConfig as a parameter of compile instead? That way we avoid passing the global around.

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.

The vite.build.target setting does not take effect in dev mode.
2 participants