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

Handle process.env #129

Merged
merged 1 commit into from Sep 30, 2024
Merged

Handle process.env #129

merged 1 commit into from Sep 30, 2024

Conversation

ghost
Copy link

@ghost ghost commented Sep 28, 2024

Something I just had to patch right now for my project. Sorry for no tests

@garronej
Copy link
Owner

Hey man thanks for the pr!

@garronej garronej merged commit 9be2b56 into garronej:main Sep 30, 2024
4 of 5 checks passed
@garronej
Copy link
Owner

And sorry for the delay

@garronej
Copy link
Owner

Oops I guess I already had a case for this:

image

@ghost
Copy link
Author

ghost commented Oct 1, 2024

Was there a reason to do it that way? I figured this was maybe a regression because using node:process was not working for me with Supabase functions.

We did switch over to something else however and I'm not sure how Deno 2.0 is going to handle stuff like this, but maybe having this be configurable would be great.

Then again, if no one ever raised this as an issue, then it may just be a special setup I was implementing (and it definitely felt "off-the-mark").

Btw, great project! It did exactly what it said it'd do 👍

@garronej
Copy link
Owner

garronej commented Oct 1, 2024

Hello @firatoezcan-2,

Was there a reason to do it that way? I figured this was maybe a regression because using node:process was not working for me with Supabase functions.

Well the reason was that there is an official polyfill for node's process. I wasn't aware that it would cause issue on some serverless engine.

We did switch over to something else however and I'm not sure how Deno 2.0 is going to handle stuff like this, but maybe having this be configurable would be great.

Regarding this specific thing, deno 2 adds process to the global so there's no need to implicitely import process from 'node:process' anymore. However it's still recomended.
https://youtu.be/DoU5DLl2Aq4?si=5wuNFFNFYgzCd2nd&t=265

Then again, if no one ever raised this as an issue, then it may just be a special setup I was implementing (and it definitely felt "off-the-mark").

Yeah, that's why I rolled back the change. It's ok to add support for something that wasn't previously supported by Denoify and yo change how things currently works.
While your proposed change might be better I'm too afraid it will break things for pepole if introduced, I can't confidently gauge the impact that it would have.

I can sugest to options for your issue:

  1. Use a .deno.ts file. Create a small file from where you export your envs like:
    env.ts
export const env = process.env;

And manually create a specific implementation for deno:
env.deno.ts

export const env = Deno.env.toObject();

And then through out your codebase you can import this file instead of using process.env directly.

  1. If refactorying your codebase in this way isn't an option you can levrage patch-package to apply the change that you made in this PR to your local Denoify install.

Btw, great project! It did exactly what it said it'd do 👍

Thank you, that's very kind of you to say. IMO the project is a bit overpromising but I'm glad it (mostly) did the trick for you.

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.

2 participants