Islands detection in a non-route file

[berlysia]

What is the feature you are proposing?

Current HasIsland component only detects islands in a route file.

If in a non-route component could detect using island, it could be something similar to "use client" in React.

We often want to use components with interactions at the end of the tree. It's not something we can immediately judge based on the route.

[yusukebe]

Hi @berlysia

Thanks for the issue.

As you may know, HasIsland is a bit loose. As you can see below, it is based on whether the file it importing is in islands/ or not.

honox/src/vite/inject-importing-islands.ts

Lines 25 to 30 in a186c42

	ImportDeclaration(path) {
	// We have to make a note that `../components/islands/foo.tsx` is also a target.
	if (path.node.source.value.includes('islands/')) {
	hasIslandsImport = true
	}
	},

We want to make sure your request comes true, so we'll try to figure out a smarter way to do it. If you do find a better way, please let us know.

[bruceharrison1984]

+1 to this.

Currently the only way to allow Island components nested within server components to function correctly is to always include the client bundle, whether an Island is present in the page or not.

[berlysia]

I have two naive options to do this:

Option 1. Calculate dependency tree from routes.

• For dynamic rendering: This is good. Calculate it, dump to JSON, read it on the rendering time.
  • How to calculate: write a plugin.
  • How to store and use the result? If we dump just a route's filename, there is a matter of handling the output of the script element.
• For SSG: This requires more investigation. I think we need additional build phase, at least with the current @hono/vite-ssg plugin.
  • Reason: we shouldn't calculate it in the same build as the client build. The generated bundle will not be optimized properly.

Option 2. Detect islands during the rendering time of each routes.

I think this is good for every use-case.
It's working on my site for now, but I suspect there might be edge cases. It depends on the order of rendering components.

// additional utility
import type { FC, PropsWithChildren } from "hono/jsx";
import { useRequestContext } from "hono/jsx-renderer";

function MarkHasIsland({ children }: PropsWithChildren) {
  // This is based on current implementation. Maybe we can use `useContext`
  const c = useRequestContext();
  c.set("__importing_islands", true);
  return <>{children}</>;
}

export function WrapForHasIslandDetection({ children }: PropsWithChildren) {
  if (import.meta.env.SSR) {
    return <MarkHasIsland>{children}</MarkHasIsland>;
  }
  return <>{children}</>;
}

// /app/islands/Foo.tsx
import { WrapForHasIslandDetection } from "./path/to/utility";

export default function Foo() {
  // do something

  return (
    <WrapForHasIslandDetection>
        {/* something */}
    </WrapForHasIslandDetection>
  )
}

// or something like this (we need additional work for island-components.ts )
export default wrapForHasIslandDetection(function Foo(){ /* something */});

[bruceharrison1984]

@berlysia I took a crack at this as well. I took a slightly different approach and just parse for dependencies during the transform phase.

I managed to get it working in the Dynamic scenario pretty easily, as well as passing a rudimentary test. The big downside is that the dependency crawling triples the transformation time of the integration.test.ts from 330ms to 1s.

I don't know what implications that may have on day-to-day DX. The method i used actually removed the need for scanning Imports via traverse entirely, yet it is still slower overall.

I'm also not sure if this would work if using tsconfig alias paths.

https://gist.github.com/bruceharrison1984/91a19ff3365cc477a9231916764fe228

[berlysia]

Hi @bruceharrison1984 , I tried your gist and it does not work on my site that uses import path mappings.
In my opinion, it would be ideal to always rely on Vite's module resolution, in the context of HonoX. I don't want to be bothered with details beyond setting up for Vite about path mapptings. That's because it's already too complicated as it is.

And it appears that precinct and dependency-tree are unable to interpret Vite's aliases 😭 .

But your approach has fewer worries than mine. It would be nice if we can rely on the Vite's module resolution mechanism...

[bruceharrison1984]

@berlysia Yeah, that doesn't surprise me.

I've been pouring over the Vite docs, and I can't seem to find any suitable mechanism that could be used to crawl the dependency graph to locate Island components. Vite transformations don't have access to other file contexts during compilation, so I'm not sure how this is managed by other libraries. If Vite keeps a dictionary of the mappings it is aware of, I have no idea how to access it.

[yusukebe]

Hi @berlysia and @bruceharrison1984

This issue should be resolved, but I think this is actually needed "at build time". That's because in many cases it is no problem to import /app/client.ts at development time, regardless of whether it has Islands or not.

So, I think that inject-importing-islands should be enabled only at build time. For that it may be possible by looking at import.meta.env.prod or by determining if the command is build in src/vite/index.ts. Since this is a build-only process, performance is not that much of an issue.

I've modified #102 by @bruceharrison1984 and pushed it now. With the PR, HasIslands supports nested components.

First, I would like to merge the PR into the "main". Next, how about including a feature to enable inject-importing-islands only at build time, as mentioned earlier?

[yusukebe]

Additions.

Then HasIslands is only enabled at build time. But that is not a problem since the user does not use it directly, but uses Script.

<Script src="/app/client.ts" async />

[bruceharrison1984]

@yusukebe I agree with your thought process, we should just make sure to note in the documentation that development mode differs (fairly significantly) from production in it's mode of operation.

[bruceharrison1984]

@yusukebe I think this one can be closed since nested components work as expected now.

[yusukebe]

@bruceharrison1984 Right!

Closing the issue. Thanks!