Skip to content

Add support for dotenv#94

Merged
JonoPrest merged 4 commits into
mainfrom
jp/add-dotenv
Jul 30, 2024
Merged

Add support for dotenv#94
JonoPrest merged 4 commits into
mainfrom
jp/add-dotenv

Conversation

@JonoPrest

Copy link
Copy Markdown
Collaborator

@JonoPrest JonoPrest requested a review from DZakh July 29, 2024 16:25
@linear

linear Bot commented Jul 29, 2024

Copy link
Copy Markdown

@DZakh DZakh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice 👍

engines: {node: '>=0.3.1'}
dev: false

/dotenv@16.4.5:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like you have pnpm v6 or something like this. Let's all use pnpm v9, so there are no conflicts with the pnpm lock file.

"dotenv": "16.4.5",
"ethers": "6.8.0",
"express": "4.19.2",
"handlers": "{{relative_path_to_root_from_generated}}",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's rename it to the root; handlers confused me when I was reviewing the code 😅

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I agree 👍🏼

@@ -1,4 +1,6 @@
// %%raw(`globalThis.fetch = require('node-fetch')`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

Comment on lines +1 to +3
type requireObj = {resolve: string => string}
external requireObj: requireObj = "require"
let _res = Dotenv.config({path: requireObj.resolve("handlers/.env")})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it needed here since you already added it to the Index.res?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I will tidy this up. It's needed because Index.res is not called when running the testing framework.

And I wanted it on Index.res since I think there is a few env var lookups there as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also realised this will error if .env does not exist so will fix that now.

@DZakh DZakh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice 🔥

@JonoPrest JonoPrest enabled auto-merge (squash) July 30, 2024 13:29
@JonoPrest JonoPrest merged commit c97a581 into main Jul 30, 2024
@JonoPrest JonoPrest deleted the jp/add-dotenv branch July 30, 2024 13:44
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