-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(script): use process.cwd() instead of __dirname #142
Conversation
nb: process.cwd() reflect the dir of the execution, so could behave differently if you exec in |
Yeah, so far, the way we've been executing scripts are not
but more like
So the result of |
scripts/pre-gen/setHostsOptions.ts
Outdated
@@ -43,7 +43,7 @@ async function setHostsOptions(): Promise<void> { | |||
throw new Error(`Generator not found: ${generator}`); | |||
} | |||
|
|||
const specPath = path.join(__dirname, `../../specs/bundled/${client}.yml`); | |||
const specPath = path.join(process.cwd(), `../specs/bundled/${client}.yml`); | |||
|
|||
if (!(await stat(specPath))) { | |||
throw new Error(`File not found ${specPath}`); |
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.
We can add make sure to use workspaces to run this script
or something similar that can give an hint
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.
(this script is ran by the generator so it might not relevant but at least give hints on why/where it fails)
@@ -32,7 +32,7 @@ type AdditionalProperties = Partial<{ | |||
}>; | |||
|
|||
async function setHostsOptions(): Promise<void> { | |||
const openapitoolsPath = path.join(__dirname, '../../openapitools.json'); | |||
const openapitoolsPath = path.join(process.cwd(), '../openapitools.json'); | |||
const openapitools = JSON.parse(await readFile(openapitoolsPath, 'utf-8')); |
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.
Can we add the same check as L48, with a similar mention to make sure to use workspaces to run the script
@shortcuts I added one comment instead of two. It was a bit weird to have the same comment within the same function. |
Sorry I was speaking about throwing an error directly and adding the reason in it. Same as this but more detailed api-clients-automation/scripts/pre-gen/setHostsOptions.ts Lines 48 to 50 in 288d07a
e.g. |
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.
Good!
* fix(scripts): use process.cwd() instead of __dirname * chore: add comment * chore: throw error when file not found
🧭 What and Why
Changes included:
The output of
yarn workspace scripts build
has changed since the latest commit. So the path of the transpiledsetHostsOptions.js
is different, which means__dirname
is now different.__dirname
is not reliable, so we should stick toprocess.cwd()
which is the working directory of the current process.