-
Notifications
You must be signed in to change notification settings - Fork 676
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
base: main
Are you sure you want to change the base?
Conversation
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. |
@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. |
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 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 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. |
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! |
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. |
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