QwikDev / astro

Qwik + Astro integration
214 stars 13 forks source link

Rollup parse error when importing a Qwik component from a barrel file that also exports an Astro component #30

Closed iivvaannxx closed 11 months ago

iivvaannxx commented 12 months ago

Context

I hope I am not being annoying with the issues, this is the third I write, it's just things that I am randomly encountering myself while using it 🥲. This time I am experimenting a very weird issue which I can't provide much context on. I was working on my project, and one thing I usually do is try building from time to time to ensure that everything works as expected and to not have any surprises. The thing is that, from nowhere, the builds started failing.

I think the integration is messing something up with Vite because it fails while transforming an Astro component that does not have any syntax errors, which has been working for almost a month and hasn't been touched since. As soon as I comment out the integration from the Astro configuration file, everything builds correctly.

Things that makes the issue very confusing

The component definition

It's just a simple wrapper of an <a> tag. As I said, no errors are given by the editor or ESLint and it works on dev.

---
import { type HTMLAttributes } from 'astro/types'
import { conditionalAttrs } from '@utils/conditionals'

export interface Props extends HTMLAttributes<'a'> {

  /** Whether the link should be prefetched if inside the viewport or not. */
  shouldPrefetch?: boolean

  /** Should the link be opened in a new tab? */
  targetIsBlank?: boolean

  /** When to underline the link. */
  underline?: 'always' | 'hover' | 'never'
}

const {

  shouldPrefetch = false,
  targetIsBlank = false,
  underline = 'always',

  class: clazz,
  ...rest

} = Astro.props

// The styles to apply based on the underline policy.
const underlineStyles = {

  underline: underline === 'always',

  'hover-underline': underline === 'hover',
  'no-underline': underline === 'never'
}

// Conditional attributes
const attrs = conditionalAttrs({

  rel: ['prefetch', shouldPrefetch],
  target: ['_blank', targetIsBlank]
})
---

<a

  uno-outline='focus-visible:2 focus-visible:white focus-visible:offset-10 focus-visible:~'
  class:list={[underlineStyles, clazz, 'rounded-0.25']} {...attrs} {...rest}
>
  <slot />
</a>

<style>

  .hover-underline {

    text-decoration: none;
  }

  .hover-underline::after {

    view-transition-name: none;

    content: '';
    width: 75%;

    position: absolute;
    inset-inline: 0;
    margin-inline: auto;
    bottom: 0;

    transform: scaleX(0);
    height: 2px;

    background-color: white;
    transform-origin: center right;
    transition: transform 0.25s ease-out;
  }

  .hover-underline:hover::after {

    transform: scaleX(1);
    transform-origin: center left;
  }

</style>

Build Log

At first I thought that it happened still because of the srcDir issue we had until the recent version, because I renamed back to source, however I tried to go back to src and it fails the same way. I even tried to rollback to 0.1.15 which still doesn't have the fix for the srcDir issue and yet the same error.

$ astro build
04:35:57 PM [content] No content directory found. Skipping type generation.
04:35:57 PM [@qwikdev/astro] astro:build:start
vite v4.5.0 building for production...
✓ 59 modules transformed.
✓ built in 359ms
[vite:build-import-analysis] Parse error @:51:5
file: /home/iivvaannxx/.../components/base/links/link.astro:51:4
49: >
50:   <slot />
51: </a>
        ^
52: 
53: <style>
transforming (64) node_modules/nanostores/index.js error   Parse error @:51:5
  File:
    /home/iivvaannxx/.../components/base/links/link.astro:51:4
  Code:
    49: >
    50:   <slot />
    51: </a>
            ^
    52: 
    53: <style>
  Stacktrace:
Error: Parse error @:51:5
    at parse$e (file:///home/iivvaannxx/.../node_modules/vite/dist/node/chunks/dep-bb8a8339.js:16497:355)
    at Object.transform (file:///home/iivvaannxx/.../node_modules/vite/dist/node/chunks/dep-bb8a8339.js:46653:27)

error: script "build" exited with code 1 (SIGHUP)

It must be the integration because as soon as I remove it from the integrations list the build succeeds.

thejackshelton commented 12 months ago

I hope I am not being annoying with the issues, this is the third I write, it's just things that I am randomly encountering myself while using it 🥲

Not at all! Happy to make the integration as awesome as possible, sorry you had to run into these!

Rollback to my previous version (0.1.15, where everything worked fine) doesn't solve anything neither.

Hm... this tells me it might not be this integration that introduced the conflict, and the error seems to be coming straight from the nanostores library.

I would try going to a previous version of astro or nanostores to see if this issue persists.


Can we call functions in a code fence on the server like this? I would comment this out to check.

// Conditional attributes
const attrs = conditionalAttrs({

  rel: ['prefetch', shouldPrefetch],
  target: ['_blank', targetIsBlank]
})

Side note: are you trying to use Nanostores with Qwik components? Because this is something we're currently working on getting to survive serialization

iivvaannxx commented 12 months ago

Not at all! Happy to make the integration as awesome as possible, sorry you had to run into these!

No worries! I like getting my hands dirty with this kind of things.

Hm... this tells me it might not be this integration that introduced the conflict, and the error seems to be coming straight from the nanostores library.

The thing is that the part where it says transforming (64) node_modules/nanostores/index.js error Parse error changes randomly, sometimes it says it's some npm module like nanostores and sometimes occurs while "transforming" a file on my project. If some other module was introducing the conflict, I wouldn't trust that specific line to know which one it is. Still, would be weird for the error to come from somewhere else when it doesn't happen if I remove the Qwik integration.

Side note: are you trying to use Nanostores with Qwik components? Because this is something we're currently working on getting to survive serialization

No, I am using nanostores, and I do have some stores declared within my source code, but they aren't still integrated into any component, neither Astro nor Qwik.

Can we call functions in a code fence on the server like this? I would comment this out to check.

I am not sure if I get what you mean 🤔, but still I tried commenting it out and the issue persists.

I've been digging for the past couple hours. So far I:

  1. Cloned the repository of this integration
  2. Copied my project into the apps folder
  3. Replaced the integration version with the one on the workspace
  4. Observed the exact same behaviour explained above
  5. Started debugging by modifying the build command of the launch.json file to build my project.

After hooking into the build process I noticed that this part of the source code is from where the error originates:

"astro:build:start": async ({ logger }) => {
  logger.info("astro:build:start");
  if ((await entrypoints).length > 0) {
    await build({ ...astroConfig?.vite }); // The error originates here.
    await moveArtifacts(distDir, tempDir);
  } else {
    logger.info("No entrypoints found. Skipping build.");
  }
}

More specifically, I was able to trace the build process down until I reached the Rollup rollupInternal function at this path (relative to the root of this repository workspace):

/node_modules/.pnpm/rollup@3.29.2/node_modules/rollup/dist/es/shared/node-entry.js:26709

This line calls graph.build() method from Rollup (to clarify, I have not a single clue of what any of this code does, but I am trying to trace the error down so far where it's originated). Which in turn invokes the following chain of methods:

this.generateModuleGraph() -> await this.moduleLoader.addEntryModules(...)

Now, this addEntryModules function receives a parameter unresolvedEntryModules which looks like the following:

image

All those id's are precisely the paths to the components I wrote using Qwik. Rollup starts to process them in this method one by one and I noticed that the builds fails specifically when processing the use-contact-form.ts hook, which looks like the following:

import { $ } from '@builder.io/qwik'
import { useForm, valiForm$ } from '@modular-forms/qwik'

import type { ContactFormSubmitHandler, UseContactFormHook } from '../lib/types'
import { ContactFormSchema, defaultValues, type ContactForm } from '../lib/schema'
import { ContactFormTextField, type ContactFormTextFieldProps } from '../components/contact-form-text-field'

/**
 * Handles the submission of a contact form.
 *
 * @param values - The values of the contact form.
 * @param event - The submit event.
*/

const submitContactForm: ContactFormSubmitHandler = async (values, event) => {

  // Retrieve the token added by Turnstile CAPTCHA.
  const form = event.target as HTMLFormElement
  const tokenField = form.querySelector('input[name="cfToken"]') as HTMLInputElement

  // Send the mail.
  await fetch('/api/contact', {

    method: 'POST',
    headers: { 'Content-Type': 'application/json' },

    body: JSON.stringify({

      cfToken: tokenField.value,
      ...values
    })
  })

  // TODO: Check response.
}

/**
 * Custom hook that returns a form and its components for the ContactForm schema.
 * @returns An array containing the form object, the submit function, and ready-to-use components.
*/

export const useContactForm: UseContactFormHook = () => {

  // Use a form with the ContactForm schema.
  const [form, { Form, Field }] = useForm<ContactForm>({

    validateOn: 'input',
    validate: valiForm$(ContactFormSchema),

    loader: { value: defaultValues }
  })

  // Wrap the Field component into a reusable TextField component.
  const TextField = (props: ContactFormTextFieldProps) => ContactFormTextField({ ...props, Field })
  return [form, $(submitContactForm), { Form, TextField }]
}

This hook code handles the creation of a contact form in my website, which is created with Modular Forms for Qwik and validated through Valibot from the same creator. I have to point out that the Qwik version of Modular Forms has "Qwik City" as a peerDependency but I managed to remove it and successfully used the library within Astro (see this issue: fabian-hiller/modular-forms#145).

In essence, it creates the form, and wraps the Field component returned by useForm into a specific JSX component named contact-form-text-field.tsx (which is not Qwik, just pure JSX). This component is not being processed by Rollup, you can check it in the screenshot above. The component looks like this:

import type { ContactFormFieldName } from '../lib/schema'
import type { ContactFormFieldComponent } from '../lib/types'

import { TextField, type TextFieldProps } from '../../../components/base' // Qwik integration Issue at #26 

// Ignore these props as they are handled by this component.
type IgnoredTextFieldProps = 'name' | 'error' | 'value' | 'required'

/** The props for a specific text field of the contact form.  */
export type ContactFormTextFieldProps = Omit<TextFieldProps, IgnoredTextFieldProps> & {

  // Enforce a key of the contact form.
  name: ContactFormFieldName
}

type Props = ContactFormTextFieldProps & {

  // The type of the 'Field' element returned by the useForm hook.
  Field: ContactFormFieldComponent
}

/** Defines a wrapper component for the TextField elements in the contact form. */
export function ContactFormTextField ({ name, Field, ...rest }: Props) {

  return (

    <Field name={name}>

      {(field, props) => (

        <TextField required

          {...rest as TextFieldProps}
          {...props}

          error={field.error}
          value={field.value}
        />
      )}

    </Field>
  )
}

It's a simple wrapper (as you can see I really like wrappers, wrappers everywhere 😂). <TextField> is a Qwik component which is the one handling the <input> field like the Modular Forms docs states here.

Ooof, this may be one of the more detailed issues I've ever written. I've still not found the exact place where the error originates but I am halfway there. I'll take a break and continue, but I wanted to share what I found and see if it rings a bell to anyone, @thejackshelton.

EDIT: Answered a couple of your questions at the beginning, which I forgot to do previously.

thejackshelton commented 12 months ago

Hm... could you try removing modular forms here just to see if it's with your own Qwik components? Like you mentioned, part of the library depends on Qwik City, which is a direct alternative to Astro.

For example, when using formAction$ in modular forms, under the hood is Qwik City's routeAction$.

iivvaannxx commented 12 months ago

has "Qwik City" as a peerDependency but I managed to remove it and successfully used the library within Astro (see this issue: https://github.com/fabian-hiller/modular-forms/issues/145)

If you look into the issue, you'll see that the creator pointed me to a workaround that helped me be able to use the library without Qwik City. Turns out that the framework is only used, as you say, within formAction$ which I don't use. So what I did was clone the library, remove the Qwik City dependencies and rebuild, then pnpm pack and I left with a custom version of the Modular Forms library that doesn't depend on Qwik City (it's completely removed from the package.json).

I actually tested and got it working, even on a build. That until today. I dug a little bit more into the use-contact-form hook, and trying commenting out a thing that I was even doubting to do because seemed a little bit "rare".

If you look at this:

/**
 * Custom hook that returns a form and its components for the ContactForm schema.
 * @returns An array containing the form object, the submit function, and ready-to-use components.
*/

export const useContactForm: UseContactFormHook = () => {

  // Use a form with the ContactForm schema.
  const [form, { Form, Field }] = useForm<ContactForm>({

    validateOn: 'input',
    validate: valiForm$(ContactFormSchema),

    loader: { value: defaultValues }
  })

 /* THIS >>> */  const TextField = (props: ContactFormTextFieldProps) => ContactFormTextField({ ...props, Field })

  return [form, $(submitContactForm), { Form, TextField }]
}

I am doing that "wrapping" on the TextField component to avoid having multiple of these:

image

That line invokes the contact-form-text-field.tsx component I've shown in the previous comment, and it does it as a function call (because at the end of the day components are just functions). It passes the "headless" Field component as a prop and it gets back a <TextField> component that is equivalent to the one on the screenshot. I didn't know how to do this at the beginning so I copied the Modular Forms library author as he does here, within the useForm hook (he does the same to return the Form and Field components).

I am not sure if that is the correct way to do what I needed but it seems to be the source of the problem, because just by removing the TextField from the return statement (and of course, removing it's usage from the app), then it builds correctly. Is this some kind of invalid syntax @thejackshelton? I don't think so because as I said at the very beginning of the issue, everything works during dev.

thejackshelton commented 12 months ago

Ah what happens if you wrap TextField in a QRL? Does it build properly?

In Qwik, a component without a component$ is an inline component, and so it's similar to components in other frameworks, but there are some restrictions. https://qwik.builder.io/docs/components/overview/#inline-components

iivvaannxx commented 12 months ago

Ah what happens if you wrap TextField in a QRL? Does it build properly?

In Qwik, a component without a component$ is an inline component, and so it's similar to components in other frameworks, but there are some restrictions. https://qwik.builder.io/docs/components/overview/#inline-components

@thejackshelton That makes sense, but unfortunately it still fails, I was really hoping that could fix it. 😞 This thing is driving me crazy and I'm out of ideas.

thejackshelton commented 12 months ago

How about trying to have TextField as a Qwik component with a component$? Then passing it all through props.

iivvaannxx commented 12 months ago

Mm.. probably we didn't understood each other. Let me clarify:

I tried to make ContactFormTextField a Qwik component, but then I can't wrap it the way I do in use-contact-form because it asks me for some arguments while invoking it as a function that I don't know which ones they should be.

iivvaannxx commented 12 months ago

Okay, I have "good" news, I think I discovered why it happens. I've been trying to isolate at the very maximum the problem, reducing and reducing the amount of code involved. Now I finally have a minimal reproduction. Here they are:

Github Repository: https://github.com/iivvaannxx/qwik-integration-bug Stackblitz Fork: https://stackblitz.com/edit/iivvaannxx-qwik-integration-bug-orpvqy?file=apps%2Fminimal-example%2Fsrc%2Fhooks%2Fuse-inline.ts

The code is now only dependant on astro, the integration itself and @builder.io/qwik, nothing more, no forms, no libraries. I created the example inside the integration's code repository, within the apps dir.

What is happening?

So I think the errors happens when the following conditions are met:

The minimal example contains a hook named use-inline.ts which the only thing it does is import from a facade which is exporting two components. Those two components are simple dummy components of either Astro and Qwik (this one is defined as an inline component, to make things simpler, but I recall the build still failing even if wrapped in a component$).

They look like this:

<!--- components/dummy-astro.astro -->
<span>Dummy Astro</span>
/* components/dummy-qwik-inline.tsx */
export default function DummyQwik() {

  return <span>Dummy Qwik</span>
}

and the facade looks like this:

// Comment this Astro re-export and the build succeeds.
export { default as DummyAstro } from './dummy-astro.astro'
export { default as DummyQwikInline } from './dummy-qwik-inline'

In a blank project of Astro, you can't do this with Astro components. But if you add the following to env.d.ts:

import 'astro/astro-jsx'
import 'astro/client'

/** Astro files export simple JSX components by default. */
declare module '*.astro' {

  const component: (props: any) => astroHTML.JSX.Element
  export default component
}

It works without any problems (most of my Astro projects are written like this, and I didn't have problems with other integrations).

Then, Vite, when is trying to build, I am guessing is trying to parse the Astro file as a tsx? I have no clue but it may be a possible reason. The hook looks like this:

import { DummyQwikInline } from '../components'

// Even if unused, the presence of this line breaks the build.
// import { component$ } from '@builder.io/qwik'

export const useInline = () => {

  // Comment this and the build succeeds.
  return DummyQwikInline()
}

If you comment the return, I think that Vite discards the import, and then everything works. But this is just a guess.

This conditions were all met in my project, but in a more subtle way, and with all the other code it was difficult to pinpoint the origin. That's the reason of why when we removed the TextField from the return it worked, the component wasn't used, the import discarded, and then the 'facade' I had wasn't being analyzed by Vite and thus no error was given.

As a final check, I wanted to figure out why the dev mode works without any problems, I thought that as the pages weren't importing the conflicting code then Vite wasn't complaining, after making the minimal example, I imported the useInline hook inside the server fence of the only Astro page the reproduction has (a simple HelloWorld page), but everything works still, so I don't have a clue what is the difference. This is the code for the page:

---
import { useInline } from '../hooks/use-inline'
const _test = useInline()
---

<!DOCTYPE html>

<html lang='en'>

  <head>
    <link rel='icon' type='image/svg+xml' href='/favicon.svg'/>
  </head>

  <body>
    <h1>Hello World!</h1>
  </body>

</html>
thejackshelton commented 12 months ago

Hey, thanks for waiting @iivvaannxx.

It seems the problem is that you are importing a Qwik hook in an Astro code fence along with exporting it as default from index.ts. Which makes sense that there is a parse error.

This pattern would not work in React for example:

image

I would instead use the Qwik hook in a Qwik component, and then use that in your Astro page somewhere.

If you still get issues let me know 👍

iivvaannxx commented 12 months ago

Hey @thejackshelton! No problem at all! It's not a blocking issue at the moment.

But I don't understand, I mean I am not doing that I think, I only use useForm hook inside a Qwik component which is contact-form.tsx. That hook is indirectly used by my custom useContactForm hook but neither is called within an Astro fence. If you say so because in my last comment I tested the useInline example hook in an Astro fence it was just a test, but I am not doing that in my project.

I mean, if you look into the stackblitz reproduction I gave you, you can safely remove the unique and only Astro code fence (located at index.astro page), and try to run pnpm run build it will still fail.

Could you clarify a bit? Maybe I did not understand correctly what you mean.

thejackshelton commented 12 months ago

Hey @thejackshelton! No problem at all! It's not a blocking issue at the moment.

But I don't understand, I mean I am not doing that I think, I only use useForm hook inside a Qwik component which is contact-form.tsx. That hook is indirectly used by my custom useContactForm hook but neither is called within an Astro fence. If you say so because in my last comment I tested the useInline example hook in an Astro fence it was just a test, but I am not doing that in my project.

I mean, if you look into the stackblitz reproduction I gave you, you can safely remove the unique and only Astro code fence (located at index.astro page), and try to run pnpm run build it will still fail.

Could you clarify a bit? Maybe I did not understand correctly what you mean.

Ah gotcha. Regardless, it seems that in Qwik you cannot return a Qwik component (or tsx) from a ts file https://qwik.builder.io/docs/components/tasks/#use-hook-rules

hooks can only be called at the root level of component$ and not inside conditional blocks or other use* methods.

I think instead you'd want to do something like this:

function useMyHook() {
  // some random logic

  return someValue;
}

export const MyQwikComponent = component$(() => {
  // Use the hook within the component
  const myValue = useMyHook();

  return (
    <div>
      {myValue}
    </div>
  );
});

useSignal, useStore, useTask$, these are all hooks.

iivvaannxx commented 12 months ago

Well I actually missed that documentation while learning Qwik (still a noob), but even so, how does that apply to the reproduction? I am not "invoking" or "using" the hook (if you remove the Astro code fence, which I just did in the stackblitz example). I am only importing it, the docs don't mention anything about that.

Stackblitz Fork: https://stackblitz.com/edit/iivvaannxx-qwik-integration-bug-orpvqy?file=apps%2Fminimal-example%2Fsrc%2Fhooks%2Fuse-inline.ts

In the example:

If the bug arises just by importing/exporting (which I don't think that's what you mean) then would be a big limitation 🤔.

thejackshelton commented 12 months ago

Well I actually missed that documentation while learning Qwik (still a noob), but even so, how does that apply to the reproduction? I am not "invoking" or "using" the hook (if you remove the Astro code fence, which I just did in the stackblitz example). I am only importing it, the docs don't mention anything about that.

Stackblitz Fork: https://stackblitz.com/edit/iivvaannxx-qwik-integration-bug-orpvqy?file=apps%2Fminimal-example%2Fsrc%2Fhooks%2Fuse-inline.ts

In the example:

* `DummyQwikInline`: A simple inline component (does not import any hook nor use it).

* `DummyAstro`: Also a simple component (does not have neither any import).

* `useInline`: It exports the hook (which returns the Qwik component).

If the bug arises just by importing/exporting (which I don't think that's what you mean) then would be a big limitation 🤔.

Ah right I think I understand now. Yeah this really is a difficult bug to track down 😅 . Thanks for your help, will try to figure out how we can get this fixed. Happens in 0.2.2 as well.

iivvaannxx commented 12 months ago

Sure it is, it is very weird. Could possibly be something with the Qwik Vite plugin? I don't have a clue of how it works, but I would guess that, as it's not thought to parse any components which aren't JSX or Qwik.

iivvaannxx commented 12 months ago

To try to help a bit, could this code block:

for await (const line of rl) {
  if (line.includes("import")) {
    importFound = true;
  }
  // This condition
  if (line.includes("@builder.io/qwik")) {
    builderFound = true;
  }
  if (importFound && builderFound) {
    qwikFiles.push(file);
    found = true;
    break;
  }
}

which is located unser libs/qwik-dev/astro/src/index.ts, have anything to do with the bug? If you remember one of the preconditions for this to happen was:

A *.ts file (in this case an example hook) imports from that facade AND it contains an import from @builder.io/qwik (even if it's not used, in the example is actually commented and still fails, however, if you remove it, it succeeds).

As I said, I am just guessing, because I am not very familiar with Qwik, but it could have something to do with it.

Side note: the bug happens even with the import commented, which I would expect it to be the same as if it wasn't there at all (in which case it does not happen), shouldn't there be another condition which checks if the line/import is commented?

thejackshelton commented 12 months ago

To try to help a bit, could this code block:

for await (const line of rl) {
  if (line.includes("import")) {
    importFound = true;
  }
  // This condition
  if (line.includes("@builder.io/qwik")) {
    builderFound = true;
  }
  if (importFound && builderFound) {
    qwikFiles.push(file);
    found = true;
    break;
  }
}

which is located unser libs/qwik-dev/astro/src/index.ts, have anything to do with the bug? If you remember one of the preconditions for this to happen was:

A *.ts file (in this case an example hook) imports from that facade AND it contains an import from @builder.io/qwik (even if it's not used, in the example is actually commented and still fails, however, if you remove it, it succeeds).

As I said, I am just guessing, because I am not very familiar with Qwik, but it could have something to do with it.

Side note: the bug happens even with the import commented, which I would expect it to be the same as if it wasn't there at all (in which case it does not happen), shouldn't there be another condition which checks if the line/import is commented?

What this part of the integration does is find which files are Qwik entrypoints, so definitely a possibility. It tries to see if it can find an import and something from builder io inside of the import.

Some extra info in the contributing guide: https://github.com/QwikDev/astro/blob/main/contributing.md

iivvaannxx commented 11 months ago

Hey @thejackshelton! I've been digging a little and trying to come up with a solution. While further investigating into the issue, I found that in the astro:build:start hook, the build function from vite where you pass down the { ...astroConfig?.vite } fails (and gives that unhelpful error) because of this transform hook from the plugin vite:build-import-analysis.

At the moment of failing, it's trying to run the parseImports method, but it does so with the source code of the .astro component. The parse fail makes sense because the Astro file contains special syntax, and I'm sure that parseImports function is expecting JS or TS code.

In fact, I managed to "solve" the issue by invoking the build method of vite by "externalizing" (in Rollup vocabulary this would mean skipping them in the bundle) the files that end with "*.astro". I did so by modifying the build hook like this:

"astro:build:start": async ({ logger }) => {
  logger.info('astro:build:start')
  if ((await entrypoints).length > 0) {

    const viteConfig = { ...astroConfig?.vite };
    const buildSettings = structuredClone(viteConfig?.build) ?? { };

    // Don't overwrite the rollupOptions.
    if (!(buildSettings.rollupOptions)) { buildSettings.rollupOptions = {} }
    const handleExternal = buildSettings.rollupOptions.external ?? null;

    // See: https://rollupjs.org/configuration-options/#external
    if (typeof handleExternal === "function" || handleExternal === null) {

      buildSettings.rollupOptions.external = (id: string, ...others) => {

        // Execute the original handler.
        if (handleExternal?.(id, ...others)) { 
          return true; 
        }

        // Ignore astro files.
        return id.endsWith(".astro")
      }
    }

    else {

      const matchers = Array.isArray(handleExternal) ? [...handleExternal] : [handleExternal];
      buildSettings.rollupOptions.external = [

        // Include the default matchers.
        ...matchers,
        "*.astro",
      ];
    }

    viteConfig.build = buildSettings;
    await build({ ...viteConfig });

    await moveArtifacts(distDir, tempDir);
  } else {
    logger.info("No entrypoints found. Skipping build.");
  }

Essentially it "extends" the external property of the rollupOptions taking into account that can be different things (I try not to overwrite the initial value):

image

Externalizing .astro files can only be done in this manual build step because otherwise the other components in the project like for example the pages won't build correctly. That's why I use structuredClone to clone the build settings recursively and not "affect" the original astro config. I am pretty sure this can be done in a more elegant way but it's what I come up with while trying. I am not sure if this is actually the solution but after doing this the build works... with a catch.

If you analyze the little q-hash files generated by Qwik, you will find this representation of the "facade" file we had:

import{default as t}from"../dummy-astro.astro";import{c as m}from"./q-Dmc0O3LN.js";export{m as CounterQwik,t as DummyAstro};

As you can see, by "skipping" Astro components, the "import" doesn't get correctly processed, and the "astro" component import is still there. This doesn't affect the application, because at least the minimal example works correctly after build, but obviously that doesn't belong there.

iivvaannxx commented 11 months ago

As I said I think the error arises because somehow an .astro file ends up so far down the build process of Vite that at some point, when it tries to parse it it fails because it's invalid syntax.

Even if the facade file doesn't get picked up during the collection of Qwik entrypoints, Vite seems to trace the imports down the line until it reaches it and then finds the Astro file. This makes sense knowing the nature of Vite. There must be some better way to tell Vite to ignore those Astro files. Perhaps by building a more precise Qwik entrypoint collection mechanism?

Currently it's just based on imports, would it be possible to trace the imports the same way Vite does so, instead of having, for example, the facade file as an entrypoint, having only the actual Qwik components exported from there as entrypoints?

This are just guesses, probably it can't be done for some reason I am not aware of or something I am not taking into account.

Final question: None of the official integrations for the other frameworks require a custom "build" step from Vite, they work just by defining the renderer in the astro:config:setup hook, do we actually need that step?

thejackshelton commented 11 months ago

Hey @iivvaannxx. Thanks for digging into this! I have been completely stumped on it.

We do want the build, because we need to build the Qwik manifest on the client, which then gets passed into the server. Other frameworks do not do this. I'm gonna include this in the contributing guide from here on.

https://qwik.builder.io/docs/guides/bundle/#waterfall

Even if the facade file doesn't get picked up during the collection of Qwik entrypoints, Vite seems to trace the imports down the line until it reaches it and then finds the Astro file. This makes sense knowing the nature of Vite. There must be some better way to tell Vite to ignore those Astro files. Perhaps by building a more precise Qwik entrypoint collection mechanism?

Do you have a React or other framework example using this facade pattern in Astro where it works? My confusion is how that affects the .ts, .tsx, and .astro files. I'd like to play with it, but I can't find any docs examples of this being used. I would like to better my understanding so that I can help in this area if possible.

hm... it seems that we are doing something unintended here with this approach. For example, if we add a counter inside of dummy-qwik-inline.tsx with the above code in the integration:

counter

export default function DummyQwik() {
  const count = useSignal<number>(0);

  return <button onClick$={() => count.value++}>{count.value}</button>;
}

we get the following error from Qwik:

Error: Code(20)
    at createAndLogError (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:47:54)
    at logErrorAndStop (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:39:57)
    at qError (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:56:12)
    at useInvokeContext (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:1490:15)
    at useSequentialScope (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:533:18)
    at useSignal (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:4400:30)
    at DummyQwik (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:4417:17)
    at useInline (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:4428:10)
    at file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/pages/index_bdeadd34.mjs:4435:3
    at index (file:///home/jackshelton/dev/playground/qwik-integration-bug/apps/minimal-example/dist/chunks/astro_5c82f92c.mjs:108:12) {
  id: 'src/pages/index.astro'
}

Perhaps by building a more precise Qwik entrypoint collection mechanism?

There has been a mention about improving this with AST's https://github.com/QwikDev/astro/pull/8. The current entry files should correctly be Qwik entry files though. If that's not the case let me know!

image

OH hold on, I see that it somehow logged out QWIK FILES: [ 'src/hooks/use-inline.ts' ] as a Qwik entry file because of this comment in use-inline.ts

// Even if unused, the presence of this line breaks the build.
// import { component$ } from '@builder.io/qwik'

I think that was actually the problem 😭 it built now. We need a way to make sure it does not count commented imports as an entry. Having this causes the problem:

// import '@builder.io/qwik'
thejackshelton commented 11 months ago

I think I have an initial AST implementation that fixes this problem

async function getQwikEntrypoints(
  dir: string,
  filter: (id: unknown) => boolean
): Promise<string[]> {
  const files = await crawlDirectory(dir);
  const qwikFiles = [];

  for (const file of files) {
    // Skip files not matching patterns
    if (!filter(file)) {
      continue;
    }

    const fileContent = fs.readFileSync(file, "utf-8");
    const sourceFile = ts.createSourceFile(
      file,
      fileContent,
      ts.ScriptTarget.ES2015,
      true
    );

    let qwikImportFound = false;

    ts.forEachChild(sourceFile, function nodeVisitor(node) {
      if (
        ts.isImportDeclaration(node) &&
        ts.isStringLiteral(node.moduleSpecifier)
      ) {
        if (node.moduleSpecifier.text === "@builder.io/qwik") {
          qwikImportFound = true;
        }
      }

      if (!qwikImportFound) {
        ts.forEachChild(node, nodeVisitor);
      }
    });

    if (qwikImportFound) {
      qwikFiles.push(file);
    }
  }

  console.log("QWIK FILES: ", qwikFiles);

  return qwikFiles;
}
iivvaannxx commented 11 months ago

We do want the build, because we need to build the Qwik manifest on the client, which then gets passed into the server.

I understand the build step now, but at the end of the day Astro is also running some kind of vite build in their build pipeline I imagine? Wouldn't that also generate the manifest? As we are adding the qwikdev plugin under the vite plugins from the Astro config.

Do you have a React or other framework example using this facade pattern in Astro where it works? My confusion is how that affects the .ts, .tsx, and .astro files.

I can make one for you if you need it, at the moment I will link this short post so you can read a bit about the pattern. Just let me know if you want to see it in an example Astro project. While searching for it I learned that is actually called a "barrel file" and not a "facade file", I will be updating the issue title after this. The essence of it is just to group imports in a single file, which then you can use a single "origin" from all your components, instead of importing them in multiple lines. It's a good way to organize them.

hm... it seems that we are doing something unintended here with this approach

Completely logical, I wasn't expecting otherwise, it was a weird way to fix the bug.

OH hold on, I see that it somehow logged out QWIK FILES: [ 'src/hooks/use-inline.ts' ]

Oh, I noticed that, but I thought that was correct because of the dependency chain, I mean, if a file depends on a Qwik component then it should be also picked as an entrypoint. But now that I think of it, may be this is what Vite already does, we should specify only Qwik components and let it bundle the dependencies accordingly. That makes sense in my head.

I think that was actually the problem 😭 it built now.

Great! It makes sense for the minimal example. However, I can think of a couple edge cases where this could fail, if you have a new version where I can test the AST I could give it a try.

I think I have an initial AST implementation that fixes this problem

If it's only a matter of looking for exports/imports you could also take a look to this package. I noticed that is what Vite uses to retrieve those specific statements. Probably would be a little less overhead that generating an entire abstract syntax tree.

As a final note:

If we require that separate build step from Vite, do we actually need to merge our Vite configuration into the vite option of the Astro config? To avoid having issues if the end user has a more complex Vite configuration I think that we can actually perform the build without merging configurations, only by creating a custom Vite configuration for that specific step should be enough. Or am I wrong?

thejackshelton commented 11 months ago

I understand the build step now, but at the end of the day Astro is also running some kind of vite build in their build pipeline I imagine? Wouldn't that also generate the manifest? As we are adding the qwikdev plugin under the vite plugins from the Astro config.

Yeah so it was Misko that added this part. My understanding is we need to do a client build that we actually "stick into" the server build, and the Astro integrations API does not give us a way to do that.

I can make one for you if you need it, at the moment I will link this short post so you can read a bit about the pattern. Just let me know if you want to see it in an example Astro project. While searching for it I learned that is actually called a "barrel file" and not a "facade file", I will be updating the issue title after this. The essence of it is just to group imports in a single file, which then you can use a single "origin" from all your components, instead of importing them in multiple lines. It's a good way to organize them.

Awesome! Thanks for linking will read through it.

Great! It makes sense for the minimal example. However, I can think of a couple edge cases where this could fail, if you have a new version where I can test the AST I could give it a try.

Yeah let me release a new version with an AST implementation, will message you here when released. Looking into the ESM lexer package you linked here too.

If we require that separate build step from Vite, do we actually need to merge our Vite configuration into the vite option of the Astro config? To avoid having issues if the end user has a more complex Vite configuration I think that we can actually perform the build without merging configurations, only by creating a custom Vite configuration for that specific step should be enough. Or am I wrong?

Yeah I'm not sure on this. If you have something in mind would love to see the approach in a PR!

thejackshelton commented 11 months ago

https://github.com/guybedford/es-module-lexer/issues/47

looks like JSX is not supported here, issue from Fred himself.

iivvaannxx commented 11 months ago

Yeah so it was Misko that added this part. My understanding is we need to do a client build that we actually "stick into" the server build, and the Astro integrations API does not give us a way to do that.

I see, that makes sense too, then if it works like this then it's just fine! I needed to clarify it so I could understand.

Yeah let me release a new version with an AST implementation, will message you here when released. Looking into the ESM lexer package you linked here too.

Good! I will try to test it tomorrow and see. And also look into that of thr Vite configuration, if it works I'll open a PR.

Too bad the lexer module doesn't support JSX, I kind of assumed it without looking as it's pretty standard these days. We will be just fine with the AST, so not a problem.

thejackshelton commented 11 months ago

Ok try 0.2.5

thejackshelton commented 11 months ago

I have also updated the contributing guide 😄 https://github.com/QwikDev/astro/blob/main/contributing.md

iivvaannxx commented 11 months ago

I have also updated the contributing guide 😄 https://github.com/QwikDev/astro/blob/main/contributing.md

Great! It's much more concise now and very well written!

Ok try 0.2.5

Now, here I come, I feel bad for ruining the party @thejackshelton . As I was suspecting switching to an AST fixed the previous minimal reproduction because the Qwik import was not being used (commented) and thus skipped by the new collection mechanism. This prevented the hook from becoming an "entrypoint", unlike the other version, where the hook was an entrypoint even though it effectively didn't use any "@builder.io/qwik" module (I mean there was the import, but it was not used at all).

However, what happens if you have a hook where you actually use a module from @builder.io/qwik? This is what happens in my real project. I've built another minimal example which is much more like my case. It works as follows:

export type Props = {

  text: string
}

export default function ({ text }: Props) {

  // Just return an inline text element.
  return <span>{text}</span>
}
import { component$ } from "@builder.io/qwik"
import { QwikInlineText } from "../components";

// Export a variant of the inline component with some default props.
export function useMyWrappedText() {

  const wrappedInlineComponent = QwikInlineText({ text: "Hello! I'm a wrapped component!" })
  return component$(() => wrappedInlineComponent)
}
import { component$ } from "@builder.io/qwik"
import { useMyWrappedText } from "../hooks/use-my-wrapped-text"

export default component$(() => {

  const WrappedText = useMyWrappedText()
  return (

    <h2>
      <WrappedText />
    </h2>
  )
})

Now this I think it's a fully equivalent example of my situation, reason why I opened this issue. We then use this final component in an Astro Page (index.astro).

---
import { QwikHeader } from '../components'
---

<html lang="en">

  <head>

    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width" />
    <meta name="generator" content={Astro.generator} />

    <link rel="icon" type="image/svg+xml" href="/favicon.svg" />
    <title>Astro</title>

  </head>

  <body>
    <h1>Hello, Qwik!</h1>
    <QwikHeader />
  </body>

</html>

If you run the build command it will fail under the same reason. What is happening is the following:

  1. The integration starts scanning for entrypoints.
  2. It finds the files use-my-wrapped-text.ts and qwik-header.tsx as entrypoints because they both contain an import statement from @builder.io/qwik.
  3. It passes them to the qwikVite plugin.
  4. The custom build step is triggered.
  5. Vite starts bundling the code, when it processes the use-my-wrapped-text.ts finds the following line:
import { QwikInlineText } from "../components";
  1. Which is traced back to the barrel file index.ts:
export { default as AstroDummy } from './astro-dummy.astro'
export { default as QwikInlineText, type Props as QwikInlineTextProps } from './qwik-inline-text'
export { default as QwikHeader } from './qwik-header'
  1. It sees the Astro component but it does not treeshake it even when it's not actually used. I think this is expected behaviour according to this section on the docs. This section explicitly recommends against the barrel files, but in my case, I only export things that I actually use, so I am not expecting to have any performance degradation.

  2. The Astro component gets down the build pipeline of Vite, until it reaches the conflicting plugin vite:build-import-analysis which fails to parse the file because of the special syntax on .astro files.

What I didn't understand is why it worked during dev. But then I remembered that dev does not trigger the custom build step, and has the empty manifest workaround. Also, I was thinking to also filter any non-tsx or non-jsx from the entrypoint collection, but I think that in this specific case Vite would end up following the dependency chain down until the same situation.

Minimal Example

Here is the reproduction I've built: https://stackblitz.com/edit/iivvaannxx-qwikdev-astro-ts6meb?file=package.json

I've added a couple of scripts astro-demo:dev (this runs on startup) and astro-demo:build on the root package.json so you can do everything without needing to cd into the directory.

thejackshelton commented 11 months ago

Hey @iivvaannxx I now have more time to look into it this week. We'll get to the bottom of it! 🕵️

iivvaannxx commented 11 months ago

Sure! If i can do anything to help just tell me @thejackshelton!

I just can't think of a solution for this that does not involve handling the build process someway different, because I don't think we can't stop Vite from finding those Astro components as it's just how it works.

thejackshelton commented 11 months ago

Sure! If i can do anything to help just tell me @thejackshelton!

I just can't think of a solution for this that does not involve handling the build process someway different, because I don't think we can't stop Vite from finding those Astro components as it's just how it works.

Yeah so I think our problem here might be that the integration is reading .astro files inside of crawlDirectory. It's doing so with the readdir method from node recursively.

I've created a new playground directory, feel free to clone it, that tries to use an alternative to readdir, and should only get js, ts, jsx, and tsx files and should specifically exclude astro files.

The build fails though, and so I am trying to figure out why 🤔 . Would really appreciate some debugging help here, a bit stumped.

iivvaannxx commented 11 months ago

@thejackshelton It reads it for sure but doesn't the function getQwikEntrypoints filter them out? From the debugging I ran in my computer what I discovered it's that it's not us who are passing the .astro component to Vite, is Vite itself discovering it.

If you run the build command it will fail under the same reason. What is happening is the following:

Did this breakdown from what I think is happening in one of my previous comments, I don't quote the whole of it because is a bit too large. But in essence, the .astro component is discovered by Vite because of an indirect dependency. Something like this:

image

You can check this whiteboard here. This is just a similar situation of what we are facing, at least how I understand it.

MyQwikFeature has an indirect dependency to MyQwikComponent because it doesn't import it directly, it imports it from the barrel file, which contains a re-export of an Astro component at the same time. Vite doesn't know that we don't use that Astro component, so it tries to parse it and fails.

thejackshelton commented 11 months ago

@thejackshelton It reads it for sure but doesn't the function getQwikEntrypoints filter them out? From the debugging I ran in my computer what I discovered it's that it's not us who are passing the .astro component to Vite, is Vite itself discovering it.

If you run the build command it will fail under the same reason. What is happening is the following:

Did this breakdown from what I think is happening in one of my previous comments, I don't quote the whole of it because is a bit too large. But in essence, the .astro component is discovered by Vite because of an indirect dependency. Something like this:

image

You can check this whiteboard here. This is just a similar situation of what we are facing, at least how I understand it.

MyQwikFeature has an indirect dependency to MyQwikComponent because it doesn't import it directly, it imports it from the barrel file, which contains a re-export of an Astro component at the same time. Vite doesn't know that we don't use that Astro component, so it tries to parse it and fails.

I've confirmed that it's directly related to Vite like you mentioned.

Where we have our build function and pass in the astro config options

Vite attempts to parse a .astro file as JavaScript, and this only happens because the .astro file is being re-exported in a barrel file.

export { default as AstroDummy } from "./astro-dummy.astro"

Now I guess the question is how do we tell this part of vite to exclude .astro files 🤔

I don't see any sort of API or option to exclude it.

thejackshelton commented 11 months ago

@iivvaannxx think we got it working here

// make sure vite does not parse .astro files
    await build({
      ...astroConfig?.vite,
      plugins: [
        ...(astroConfig?.vite.plugins || []),
        {
          enforce: "pre",
          name: "astro-noop",
          load(id) {
            if (id.endsWith(".astro")) {
              return "export default function() {}";
            }
          },
        },
thejackshelton commented 11 months ago

try out 0.3.1. Works on my end. Closing.

Happy to re-open if you're experiencing the same issue, otherwise let's make a new one.

iivvaannxx commented 11 months ago

@thejackshelton I mean, wow I am surprised than a problem like this one could have such a simple solution. Adding a way to ignore the astro files was something I tried but failed doing so in the way it needed to be done. I am glad though because it also works on my end! :partying_face:

As a test I tried returning an empty string instead of an empty function and it also worked, so it could probably be a more accurate return value.

thejackshelton commented 11 months ago

@thejackshelton I mean, wow I am surprised than a problem like this one could have such a simple solution. Adding a way to ignore the astro files was something I tried but failed doing so in the way it needed to be done. I am glad though because it also works on my end! 🥳

As a test I tried returning an empty string instead of an empty function and it also worked, so it could probably be a more accurate return value.

Woohoo! This was a wild ride haha.

Hm... what do we think would be best there then 😅 . I guess an empty string would be best since we don't need the extra function?

iivvaannxx commented 11 months ago

I think it could be a better option yes. I am not familiar with how this load callback works but what I first thought when I saw this code is that for every Astro component Vite would write an empty function to the bundle.

I am not sure if this is what would happen but an empty string seems more safe to me for that reason @thejackshelton.