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

feat(islands): Allow importing islands outside of project with hint comment #1997

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikehw
Copy link

@mikehw mikehw commented Nov 2, 2023

This PR introduces a way to import islands from outside of the islands or routes folder, including URL based imports, by adding a @fresh-island hint in the line above the import statement. The relative path or URL of the island will be used to build the javascript variable name.

This means that you will no longer need to create placeholder files in your islands directory if you want to import an island from a URL or share islands across projects.

Example

// @fresh-island
import { Chart } from "https://deno.land/x/fresh_charts@0.3.1/island.tsx";
// @fresh-island
import { SharedIsland } from "../../packages/shared-islands/shared-island.tsx";

export default function Home() {
  return (
    <div>
      <Chart {...props} />
      <SharedIsland />
    </div>
  );
}
@mikehw mikehw changed the title feat(islands): Allow importing islands without needing an island file Nov 2, 2023
@mikehw mikehw changed the title feat(islands): Allow importing islands outside of islands folder with hint comment Nov 2, 2023
@mikehw
Copy link
Author

mikehw commented Nov 2, 2023

I thought this hint approach would be cleaner than listing external islands in FreshConfig. It uses RegEx instead of a parser to find the hints and parse the imports, but it seems to work.

@guifromrio
Copy link

@marvinhagemeister this would be a game-changer for deno-compose: it's the only thing preventing us from offering a full-blown web-based code playground with support for islands. We would love to contribute on this if possible.

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Nov 7, 2023

I'm very much in favour of having the ability to mark imports as being islands. This topic came up a few times internally as well. I'm not sure if the approach chosen in this PR is a good long term one though. Giving comments in JS special meaning feels wrong and makes the code more noisy. JS supports import attributes and that seems to be a more JS-like place to add metadata to an import. It would also make it easier for potential future static analysis of code as there is usually no comment visitor callback compared to the other AST nodes.

Approach in this PR:

// @fresh-island
import Foo from "https://example.com/Foo.tsx";
// @fresh-island
import Bar from "https://example.com/Bar.tsx";
// @fresh-island
import Baz from "https://example.com/Baz.tsx";

vs import attributes, similar to .json imports:

import Foo from "https://example.com/Foo.tsx" with { fresh: "island" };
import Bar from "https://example.com/Bar.tsx" with { fresh: "island" };
import Baz from "https://example.com/Baz.tsx" with { fresh: "island" };

From what I can tell the PR changes the way we collect islands by searching from the project root dir instead of _islands. This has a direct impact on startup time as module folders like node_modules or deno's vendor often grow to +10k files, same for other temporary folders in a project.

A more reliable approach that has less of an impact on startup performance is to treat each route file as an entry point and walk the module graph, which is much shorter. This can be done with https://github.com/guybedford/es-module-lexer and that supports import attributes as well. Ultimately, we'll need to keep track of the module graph sooner or later to support HMR anyway. It's something I was planning on tackling in the coming days.

@guifromrio
Copy link

That's a great distinction, which I haven't noticed myself. I completely agree with you. If you are going to tackle this, amazing. If you need some help from our team, we would also be glad to chip in. Either way, looking forward to this milestone!

@mikehw
Copy link
Author

mikehw commented Nov 8, 2023

Played around locally using es-module-lexer to parse routes for imports. The package doesn't have plans to support JSX (guybedford/es-module-lexer#47), so I used emit to transpile the route TSX/JSX to javascript, but ran into an issue where emit needs to know the import map used by the module. I was hoping there might be a package for loading the correct import map for a file since that logic might grow as deno evolves, but I couldn't find one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants