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

JSX refactor #7924

Merged
merged 26 commits into from
Aug 11, 2023
Merged

JSX refactor #7924

merged 26 commits into from
Aug 11, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 2, 2023

Changes

  • Remove custom babel setup to support multiple JSX frameworks.
  • Instead use each framework's own Vite plugins.
  • To support multiple, new include/exclude objects lets you target only certain files.
  • That's the only breaking change.

Testing

  • We had an existing test fixture for this, so it's just updated.

Docs

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

🦋 Changeset detected

Latest commit: 7626af2

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 pkg: react Related to React (scope) pkg: preact Related to Preact (scope) pkg: solid Related to Solid (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Aug 2, 2023
package.json Outdated Show resolved Hide resolved
@@ -95,7 +94,7 @@ export function createBaseSettings(config: AstroConfig): AstroSettings {
},
},
],
renderers: [jsxRenderer],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer added by default, only used by the mdx plugin.

@FredKSchott
Copy link
Member

Amazing! This will be so huge for anyone building more complex interactive islands/SPAs with Astro!

What happens if a JSX file is unmatched/unclaimed by any integration? Is it possible that I could have one integration act as the "default"? This would map to a user who uses React for everything, except for a couple of Solid or Preact components that they have on the page.

// Example:
// src/components/preact/*.jsx (the special preact components)
// src/components/**/*.jsx     (the rest of your codebase)
export default defineConfig({
  integrations: [
    preact({include: 'src/components/preact/**/*.jsx'}),
    react(),
  ]
})

@matthewp
Copy link
Contributor Author

matthewp commented Aug 3, 2023

I'm pretty sure this already works, if you don't use an include the Vite plugins don't filter anything out. So you need to put that integration last. I would expect your example to work as written. So i think this is a documentation thing.

@bluwy
Copy link
Member

bluwy commented Aug 3, 2023

The example would work, but probably not ideal because the preact JSX file will be transformed twice by Preact and React plugins? It would be better to explicitly configure the includes.

@matthewp
Copy link
Contributor Author

matthewp commented Aug 3, 2023

@bluwy You mean for the Preact plugin? The React pass will occur after the JSX is already compiled away. So it's just a noop. Is that what you mean?

Also, from my debugging, the React plugin (and possibly others) only run Babel when there's certain configuration to do so, otherwise it falls back to the esbuild plugin.

@bluwy
Copy link
Member

bluwy commented Aug 3, 2023

Yeah it's the noop that I'm concerned, since there's still effort and perf cost there to process it.

@FredKSchott
Copy link
Member

Cool, thanks for checking!

@matthewp matthewp marked this pull request as ready for review August 10, 2023 12:50
@matthewp matthewp requested a review from a team as a code owner August 10, 2023 12:50
@matthewp
Copy link
Contributor Author

This is ready for review. The lint error shown is wrong, eslint is drunk in CI (fine locally).

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.

Code looks great, super excited for this! Just a few small cleanup things.

packages/astro/src/core/create-vite.ts Outdated Show resolved Hide resolved
packages/astro/src/core/create-vite.ts Outdated Show resolved Hide resolved
import type { LogOptions } from '../core/logger/core.js';
import type { PluginMetadata } from '../vite-plugin-astro/types';

import babel from '@babel/core';
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can drop @babel/core if this is the only place we're using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In favor of esbuild you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

esbuild already runs. I'm not sure what implications there are if we remove this code to be honest.

Copy link
Member

@ematipico ematipico Aug 11, 2023

Choose a reason for hiding this comment

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

I think the issue is that we have this tagExportsPlugin that is probably built to use babel, and maybe we cannot convert it to an esbuild plugin. Maybe let's explore it in another PR

packages/integrations/solid/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/solid/src/index.ts Outdated Show resolved Hide resolved
.changeset/perfect-horses-tell.md Outdated Show resolved Hide resolved
.changeset/perfect-horses-tell.md Outdated Show resolved Hide resolved
@natemoo-re
Copy link
Member

Also could we add // eslint:disable @typescript-eslint/no-unused-vars to the astro/packages/astro/src/core/build/plugins/plugin-analyzer.ts:1:23 file to silence that lint error?

packages/astro/src/vite-plugin-mdx/index.ts Show resolved Hide resolved
packages/integrations/preact/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/react/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/solid/src/index.ts Outdated Show resolved Hide resolved
@matthewp
Copy link
Contributor Author

I think I've addressed everything I can here, would appreciate another review when people are ready.

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.

Some minor nits, but looks good overall!

packages/integrations/solid/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/solid/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/react/src/index.ts Outdated Show resolved Hide resolved
packages/integrations/preact/src/index.ts Outdated Show resolved Hide resolved
@matthewp matthewp merged commit 519a1c4 into next Aug 11, 2023
@matthewp matthewp deleted the new-jsx branch August 11, 2023 14:05
@bluwy bluwy mentioned this pull request Sep 5, 2023
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: example Related to an example package (scope) pkg: integration Related to any renderer integration (scope) pkg: preact Related to Preact (scope) pkg: react Related to React (scope) pkg: solid Related to Solid (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants