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

➕ Add Deno adapter demo (#75) #77

Merged
merged 2 commits into from
Mar 29, 2024
Merged

Conversation

siguici
Copy link
Collaborator

@siguici siguici commented Mar 28, 2024

I just added a demo that uses Astro's Deno adapter and I encountered no errors either for the dev script or for the build script as mentioned in #75.

@thejackshelton
Copy link
Member

Our main use case for process.env is to communicate between server.ts and index.ts in the integration.

If I recall Arsh from the core team mentioned something about a vite global that could be used instead a while back 🤔

},
"dependencies": {
"@astrojs/check": "^0.5.10",
"@astrojs/deno": "^5.0.1",
Copy link

@Mac-Mann Mac-Mann Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @siguici Thanks for taking on this issue! I have been out sick for the past few days and am just getting caught up with this discussion.

Not sure if you have seen either of these comments?

denoland/deno-astro-adapter#10 (comment)
denoland/deno-astro-adapter#13

Given the outdated NPM registry, might want to change this to:

Suggested change
"@astrojs/deno": "^5.0.1",
"@astrojs/deno": "github:denoland/deno-astro-adapter",

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @siguici Thanks for taking on this issue! I have been out sick for the past few days and am just getting caught up with this discussion.

Not sure if you have seen either of these comments?

Wishing you a speedy recovery.

I've just seen the issue, but I don't think the problem lies there, considering that Astro's integration with Deno doesn't seem to be an issue. The problem seems to be more related to the use of environment variables. Deno doesn't support process.env.
However, I'll try to explore this way to see if it resolves the problem.

@siguici
Copy link
Collaborator Author

siguici commented Mar 28, 2024

Our main use case for process.env is to communicate between server.ts and index.ts in the integration.

I'm aware of that, and I believe it's the only way at the moment to achieve this. Until we find a better solution.

If I recall Arsh from the core team mentioned something about a vite global that could be used instead a while back 🤔

I know it's possible to use environment variables from Vite's user configuration, but I'm not sure if there's a way to pass them globally through Vite since it's intended for frontend use and not server-side. So I think it's possible to do it in the src/index.ts file, but for the server.ts file, we'll have to use the method suggested by the server-side runtime.

@thejackshelton
Copy link
Member

thejackshelton commented Mar 28, 2024

Good catch @Mac-Mann! Also @siguici I reviewed the PR and it looks good! Maybe we should change the names of the demo's to: deno-demo and node-demo.

#77 (comment)

what do we think about the suggested change here? I've heard of the issues with Astro and Deno up to this point.

Edit: I tried building with the proposed package change above.

Build errored with:

<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0xc9e850 node::Abort() [node]
 2: 0xb720ff  [node]
 3: 0xec1a70 v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 4: 0xec1d57 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, v8::OOMDetails const&) [node]
 5: 0x10d3dc5  [node]
 6: 0x10d4354 v8::internal::Heap::RecomputeLimits(v8::internal::GarbageCollector) [node]
 7: 0x10eb244 v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GarbageCollectionReason, char const*) [node]
 8: 0x10eba5c v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 9: 0x10edbba v8::internal::Heap::HandleGCRequest() [node]
10: 0x1058fd7 v8::internal::StackGuard::HandleInterrupts() [node]
11: 0x14fab32 v8::internal::Runtime_StackGuardWithGap(int, unsigned long*, v8::internal::Isolate*) [node]
12: 0x7f014de99ef6 
Aborted

@siguici
Copy link
Collaborator Author

siguici commented Mar 28, 2024

Good catch @Mac-Mann! Also @siguici I reviewed the PR and it looks good! Maybe we should change the names of the demo's to: deno-demo and node-demo.

Thank you. That's a very good suggestion.

Yes, I tried the same thing and it also failed.

I'm making the suggested changes in the PR.

@thejackshelton
Copy link
Member

@siguici think it's good to merge? Looks good to me!

@siguici
Copy link
Collaborator Author

siguici commented Mar 29, 2024

@siguici think it's good to merge? Looks good to me!

Yes, it can be merged without any issues.

@thejackshelton thejackshelton merged commit ea64390 into QwikDev:main Mar 29, 2024
@thejackshelton
Copy link
Member

Merged :)

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

Successfully merging this pull request may close these issues.

3 participants