Make sure to import 'dirname' before using it#2781
Make sure to import 'dirname' before using it#2781tyler-breisacher-zipline wants to merge 1 commit intoaspect-build:mainfrom
Conversation
|
|
|
|
|
| import { join } from 'node:path' | ||
| import { dirname, join } from 'node:path' | ||
|
|
||
| const appDir = import.meta.dirname || dirname(fileURLToPath(import.meta.url)) |
There was a problem hiding this comment.
| const appDir = import.meta.dirname || dirname(fileURLToPath(import.meta.url)) | |
| const appDir = import.meta.dirname |
or, would you rather just do this?
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a52955bd24
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -1,4 +1,4 @@ | |||
| import { join } from 'node:path' | |||
| import { dirname, join } from 'node:path' | |||
There was a problem hiding this comment.
fileURLToPath alongside the new dirname fallback
In runtimes where import.meta.dirname is undefined, this file still evaluates dirname(fileURLToPath(import.meta.url)), but fileURLToPath is never imported. That means the example continues to fail with ReferenceError: fileURLToPath is not defined on those Node versions, so this change does not actually fix the portability problem it is trying to address.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ok same question, should we import fileURLToPath or should we just get rid of the stuff after the ||?
2 participants
I was following this example in our internal repo and Greptile caught this error -- we were using dirname without importing it.
Changes are visible to end-users: no
Test plan