Skip to content

refactor(fs): use fs instead of fs-extra in copyFiles. #5761

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Oct 16, 2024
Merged

refactor(fs): use fs instead of fs-extra in copyFiles. #5761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bb7d90c
move over to fs
Hweinstock Oct 9, 2024
823d6f7
add type signature
Hweinstock Oct 9, 2024
54ae361
switch to cpSYnc
Hweinstock Oct 9, 2024
f246422
move into main function
Hweinstock Oct 9, 2024
19525fe
add experimental node flag
Hweinstock Oct 10, 2024
1b5ff35
adjust flag, add techdebt test
Hweinstock Oct 10, 2024
2fd7868
change flag
Hweinstock Oct 10, 2024
5256feb
Merge branch 'master' into updateScripts
Hweinstock Oct 10, 2024
7ed0296
fix typo
Hweinstock Oct 10, 2024
21f1728
make the flags contained in bash file
Hweinstock Oct 10, 2024
9c23639
remove experimental flag
Hweinstock Oct 10, 2024
5b72dd0
try type assertions
Hweinstock Oct 10, 2024
1bd5782
Merge branch 'master' into updateScripts
Hweinstock Oct 16, 2024
7c5e40f
add dependency
Hweinstock Oct 16, 2024
be2e88d
update package-lock
Hweinstock Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion buildspec/shared/linux-pre_build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ if [ "$TOOLKITS_CODEARTIFACT_DOMAIN" ] && [ "$TOOLKITS_CODEARTIFACT_REPO" ] && [
fi

# TODO: move this to the "install" phase?
export NODE_OPTIONS=--max-old-space-size=8192
export NODE_OPTIONS='--max-old-space-size=8192'
npm 2>&1 ci | run_and_report 2 'npm WARN deprecated' 'Deprecated dependencies must be updated.'
18 changes: 15 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"webpack-merge": "^5.10.0"
},
"dependencies": {
"@types/node": "^22.7.5",
"vscode-nls": "^5.2.0",
"vscode-nls-dev": "^4.0.4"
}
Expand Down
26 changes: 14 additions & 12 deletions packages/amazonq/scripts/build/copyFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

/* eslint-disable no-restricted-imports */
import * as fs from 'fs-extra'
import fs from 'fs'
import * as path from 'path'

// Moves all dependencies into `dist`
Expand Down Expand Up @@ -73,33 +73,35 @@ const tasks: CopyTask[] = [
},
]

async function copy(task: CopyTask): Promise<void> {
function copy(task: CopyTask): void {
const src = path.resolve(projectRoot, task.target)
const dst = path.resolve(outRoot, task.destination ?? task.target)

try {
await fs.copy(src, dst, {
fs.cpSync(src, dst, {
Copy link
Contributor

@justinmk3 justinmk3 Oct 16, 2024

Choose a reason for hiding this comment

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

We could avoid the formal dependency on @types/node@22 by casting this to any. We normally wouldn't do that, but it may be fine in this case because it avoids confusion, since our packages/ depend on @types/node@16 (and that's intentional because vscode ships with node 16).

We can add a check to techdebt.test.ts to remind us to revisit this when we bump to node 18:

aws-toolkit-vscode/packages/core/src/test/techdebt.test.ts

Lines 30 to 35 in 6fa3a9e

it('nodejs minimum version', async function () {
const minNodejs = env.getMinNodejsVersion()
// XXX: available since node 16, but not sure how much work this will be, yet.
assert.ok(
semver.lt(minNodejs, '18.0.0'),

Suggested change
fs.cpSync(src, dst, {
(fs as any).cpSync(src, dst, {

Copy link
Contributor Author

@Hweinstock Hweinstock Oct 16, 2024

Choose a reason for hiding this comment

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

I see, so once we update to 18, we will have @types/node@18 which should allow us to remove the any?

Also, tracking as a follow-up.

recursive: true,
overwrite: true,
force: true,
errorOnExist: false,
})
} catch (error) {
throw new Error(`Copy "${src}" to "${dst}" failed: ${error instanceof Error ? error.message : error}`)
}
}

void (async () => {
const args = process.argv.slice(2)
if (args.includes('--vueHr')) {
vueHr = true
console.log('Using Vue Hot Reload webpacks from core/')
}
const args = process.argv.slice(2)
if (args.includes('--vueHr')) {
vueHr = true
console.log('Using Vue Hot Reload webpacks from core/')
}

function main() {
try {
await Promise.all(tasks.map(copy))
tasks.map(copy)
} catch (error) {
console.error('`copyFiles.ts` failed')
console.error(error)
process.exit(1)
}
})()
}

void main()
2 changes: 1 addition & 1 deletion packages/amazonq/test/e2e/amazonq/framework/jsdomInjector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,5 @@ export function injectJSDOM() {
})

// jsdom doesn't have support for structuredClone. See https://github.com/jsdom/jsdom/issues/3363
global.structuredClone = (val) => JSON.parse(JSON.stringify(val))
global.structuredClone = (val: any) => JSON.parse(JSON.stringify(val))
}
17 changes: 9 additions & 8 deletions packages/core/scripts/build/copyFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

/* eslint-disable no-restricted-imports */
import * as fs from 'fs-extra'
import fs from 'fs'
import * as path from 'path'

// Moves all dependencies into `dist`
Expand Down Expand Up @@ -46,27 +46,28 @@ const tasks: CopyTask[] = [
},
]

async function copy(task: CopyTask): Promise<void> {
function copy(task: CopyTask): void {
const src = path.resolve(projectRoot, task.target)
const dst = path.resolve(outRoot, task.destination ?? task.target)

try {
await fs.copy(src, dst, {
fs.cpSync(src, dst, {
recursive: true,
overwrite: true,
force: true,
errorOnExist: false,
})
} catch (error) {
throw new Error(`Copy "${src}" to "${dst}" failed: ${error instanceof Error ? error.message : error}`)
}
}

void (async () => {
function main() {
try {
await Promise.all(tasks.map(copy))
tasks.map(copy)
Copy link
Contributor

@justinmk3 justinmk3 Oct 16, 2024

Choose a reason for hiding this comment

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

It's a bit surprising that we were running copy tasks in parallel. I doubt that gives much of a speedup, and it makes the logs less readable, and it could lead to weird races. I think this is better now.

} catch (error) {
console.error('`copyFiles.ts` failed')
console.error(error)
process.exit(1)
}
})()
}

void main()
17 changes: 9 additions & 8 deletions packages/toolkit/scripts/build/copyFiles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

/* eslint-disable no-restricted-imports */
import * as fs from 'fs-extra'
import fs from 'fs'
import * as path from 'path'

// Copies various dependencies into "dist/".
Expand Down Expand Up @@ -100,27 +100,28 @@ const tasks: CopyTask[] = [
},
]

async function copy(task: CopyTask): Promise<void> {
function copy(task: CopyTask): void {
const src = path.resolve(projectRoot, task.target)
const dst = path.resolve(outRoot, task.destination ?? task.target)

try {
await fs.copy(src, dst, {
fs.cpSync(src, dst, {
recursive: true,
overwrite: true,
force: true,
errorOnExist: false,
})
} catch (error) {
throw new Error(`Copy "${src}" to "${dst}" failed: ${error instanceof Error ? error.message : error}`)
}
}

void (async () => {
function main() {
try {
await Promise.all(tasks.map(copy))
tasks.map(copy)
} catch (error) {
console.error('`copyFiles.ts` failed')
console.error(error)
process.exit(1)
}
})()
}

void main()
Loading