-
-
Notifications
You must be signed in to change notification settings - Fork 62
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: Jsx import source #301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I’ve tested in local machine and it worked as normally 😁
Heys guys are we ready to merge it? @nyannyacha @andreespirela |
I don't know If I did something wrong, but I couldn't make it work using this branch. Do you have a working example? |
Tested with a fresh project but it did not work. Then I tried to rewrite the entire source code adding jsx pragma and it worked. So I think there are some missing configuration yet |
Note:
|
This is interesting
|
After modifying dev.ts -> index.ts
main.ts -> index.ts
|
Command
// examples/fresh/fresh.gen.ts
...
const manifest = {
routes: {
"./routes/_404.tsx": $_404,
"./routes/_app.tsx": $_app,
"./routes/api/joke.ts": $api_joke,
"./routes/greet/[name].tsx": $greet_name_,
"./routes/index.tsx": $index,
},
islands: {
"./islands/Counter.tsx": $Counter,
},
// baseUrl: import.meta.url
baseUrl: "file://./mnt/data/examples/fresh/fresh.gen.ts", // <- You will not able to make it a relative path whatever you do
} satisfies Manifest;
export default manifest; import {
dirname,
fromFileUrl,
} from "https://deno.land/[email protected]/path/mod.ts";
console.log(import.meta.url);
console.log(dirname(fromFileUrl("file://./mnt/data/examples/fresh/fresh.gen.ts")));
// Note that this is an ABSOLUTE path.
console.log(Deno.readTextFileSync("/mnt/data/examples/fresh/deno.json"));
// Note that this is a RELATIVE path.
console.log(Deno.readTextFileSync("./mnt/data/examples/fresh/deno.json"));
😅 |
Sorry, I forgot to mention that I did a few modifications to make it work as is. First, for sake of simplicity, I've removed the static VFS and switched to DenoCompileFileSystem, second, I set the import map inline so those imports will be resolved automatically, @nyannyacha @andreespirela want me to setup a reproducible repo? |
By the way, we won the product of the week/day at producthunt.com https://www.producthunt.com/posts/deco-cx-2-0 And I saw that you guys posted earlier today ours, amazing!! So our chance to get the product of the month walked away, but nice to have you guys as the one that will win this month. Good luck, you have my vote! And it will be yet nicer to have you guys a deep integration with us. so running fresh sites is only the first step! |
Hello @mcandeia 😁
Actually, I posted the above not only for you, but also for andres.
Thank you! |
Yes, That will help 😁 |
I can certainly do that but it's not working e2e, somehow its failing when trying to compile .tsx using the specified JsxImportSource, I have no idea why, needs some deeper investigation. But let me prepare a reproducible repo tomorrow and I'll send it back here |
Guys @nyannyacha and @andreespirela, check out this PR: deco-cx#6 try out yourself, I did the modifications mentioned before and added a |
btw, I think this is not a semver compliant version, that should be the why |
Yeah, I didn't realise that property was being parsed as semver by other packages. 😓 |
(FYI, I am at GMT+9 and start my activity around this time every day. I will now go through your reproducible repository 😋) |
Update: I've found the problematic line. edge-runtime/crates/sb_graph/graph_util.rs Lines 333 to 334 in 7014052
--- a/crates/sb_graph/graph_util.rs
+++ b/crates/sb_graph/graph_util.rs
@@ -330,7 +330,7 @@ pub async fn create_eszip_from_graph_raw(
let parser_arc = emitter.clone().parsed_source_cache().unwrap();
let parser = parser_arc.as_capturing_parser();
- eszip::EszipV2::from_graph(graph, &parser, Default::default())
+ eszip::EszipV2::from_graph(graph, &parser, emitter.emit_options())
}
pub async fn create_graph(
I've confirmed that this patch is rendering static HTML content correctly on @mcandeia's PR-6 without file system sandboxing and static fs applied. Output:
Note: This issue also affects a decorator PR I recently submitted. |
Amazing! Will test soon. Just to clarify one thing, do I need to set the file system to be the Deno's Compile one? or there are a command that make it work out of the box? |
Oh wow, sorry, I'm not a native, so I guess that sentence was ambiguous. 😂 (However, I've only checked with Postman, so I can't guarantee that everything works for the fresh example) |
Oh, I mean, I had to change the following line: https://github.com/deco-cx/edge-runtime/pull/6/files#diff-cce0097be8934b699189157e13e2c8eed2421b37691ab0d0cb25640dcc0f741bL369 is it still necessary or there is a command to link the static file system (like --static *)? To be honest, it would be amazing to have a feature to pass the filesystem inline as a hashmap {"FILE_PATH": "FILE_CONTENT_BYTES" } |
@nyannyacha Want me to open a PR with the modifications you mentioned plus this one? I will be glad to have my first contribution |
Yes I would be happy for you to do that 😁 But in the case of static fs modification PR, it was written by @andreespirela, so I think it would be better to hear his thoughts first. |
Hey @andreespirela any thoughts? |
@mcandeia We're discussing the how-to internally. It doesn't make sense to disable StaticFS, and changing |
If I can share my thoughts: I would love to have a way to pass the filesystem inline as a hashmap. Or even as a "lazy" structure. So we can have two separate things:
This is needed since fresh has build files that needs to be included in the generated eszip but they are not directly imported from the code and they are "lazily loaded" My 50 cents |
Hello guys @andreespirela @nyannyacha, any news? |
static fs is fixed mounted to ./mnt/data
currently, so some packages can’t resolve their sub dirs/files using import.meta.url
.
Guys @laktek @andreespirela and @nyannyacha, I think the scope of this PR now has changed completely, initially it should be only for supporting JSX import source and I think it was done, just need to fix the missing emitter options as @nyannyacha said. Ins't better to just create a separate issue for tracking filesystem-related things? Running fresh projects is out of scope of this PR, it was just an example, supporting JSX is a good start for that |
Yeah, now that I think about it, this was PR for JSX support. I think I've gotten away from the point of this PR, as I thought the fresh package case was the main use case. So I think you're right, we need to separate the static fs issue. |
2fc9676
to
b71c34a
Compare
🎉 This PR is included in version 1.52.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.