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

Hweinstock · 2024-10-09T20:42:37Z

Problem

(title)

Solution

Based on answer here: https://stackoverflow.com/questions/13786160/copy-folder-recursively-in-node-js we can use cp. This assumes node v16.7.0+.
Note that overwrite is now force based on https://nodejs.org/api/fs.html#fscpsyncsrc-dest-options

For this to work, we must bump the dependency @types/node. Currently bumped to latest, but can try to test earliest version that works if that would be desirable.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

packages/amazonq/scripts/build/copyFiles.ts

Hweinstock · 2024-10-09T22:28:32Z

I'm tempted to consolidate the shared logic among copyFiles somewhere, but its spanning across the extensions. Is it worth leaving it as duplicated code to avoid cross-extension imports?

justinmk3 · 2024-10-10T18:22:44Z

Needed to add --experimental-modules flag to node runtime for it to work in CI. Added a tech debt test to remove this once its not longer experimental.

👍 there might be way to enable that flag only for the npm package.json copyFiles task.

I'm tempted to consolidate the shared logic among copyFiles somewhere, but its spanning across the extensions. Is it worth leaving it as duplicated code to avoid cross-extension imports?

It's worth creating a tracking issue but might involve some tradeoffs, so better to skip it for now.

Hweinstock · 2024-10-10T21:47:13Z

Unfortunately the flag didn't make it work in CI, removed the outdated piece in the description.

justinmk3 · 2024-10-11T21:40:38Z

TSError: ⨯ Unable to compile TypeScript:
Error: scripts/build/copyFiles.ts(108,12): error TS2339: Property 'cpSync' does not exist on type 'typeof import("fs")'.

/Users/runner/runners/2.320.0/externals/node20/bin/node: --experimental-fs is not allowed in NODE_OPTIONS

yeah I don't see a relevant flag at https://nodejs.org/dist/latest-v16.x/docs/api/cli.html#cli_node_options_options

A way to check different node versions is to use https://github.com/nvm-sh/nvm and then try fs.cpSync on 16/18/20.

Hweinstock · 2024-10-14T15:01:19Z

Yeah I used nvm to downgrade it locally to the same in CI, and it started working. Still unsure what it is about CI that makes it different.

justinmk3 · 2024-10-14T20:34:51Z

Still unsure what it is about CI that makes it different.

Let's try bumping GHA CI to node 18 (or start with 22 and work backwards):

aws-toolkit-vscode/.github/workflows/node.js.yml

Line 65 in 4df3106

node-version: [16.x]

If node 18 works, we can use that in all CI. Or even newer. There's no real reason why the node used in CI needs to match the node shipped with vscode; the vscode-shipped node will be used to execute the actual test code.

Hweinstock · 2024-10-16T15:48:37Z

I made @types/node a direct dependency, which I believe consequently bumped its version and fixed the error.

justinmk3 · 2024-10-16T21:13:55Z

packages/amazonq/scripts/build/copyFiles.ts


    try {
-        await fs.copy(src, dst, {
+        fs.cpSync(src, dst, {


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, {

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.

justinmk3 · 2024-10-16T21:17:26Z

packages/core/scripts/build/copyFiles.ts

+function main() {
    try {
-        await Promise.all(tasks.map(copy))
+        tasks.map(copy)


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.

justinmk3 · 2024-10-16T21:20:38Z

Nice! Can we remove fs-extra now? Or could be a followup.

npm uninstall --save '@types/fs-extra' 'fs-extra' -w packages/core/

justinmk3 · 2024-10-16T21:31:45Z

Merged. Let's do the above comments as a followup, that way if there are any problems it's easy to revert or drop.

## Problem Follow up from #5761 (comment). ## Solution `npm uninstall --save '@types/fs-extra' 'fs-extra' -w packages/core/`

## Problem Follow up to: #5761 (comment) ## Solution - target a version of `@types/node` that is closer to the actual version running in CI. - add a techdebt.test.ts reminder.

## Problem Eliminate fs-extra from the codebase. ## Solution We can use `fs.cp` since node v16.7.0+. Note that `overwrite` is now `force` based on https://nodejs.org/api/fs.html#fscpsyncsrc-dest-options Bump the dependency @types/node to avoid casting (fs as any).

## Problem Follow up from aws#5761 (comment). ## Solution `npm uninstall --save '@types/fs-extra' 'fs-extra' -w packages/core/`

## Problem Follow up to: aws#5761 (comment) ## Solution - target a version of `@types/node` that is closer to the actual version running in CI. - add a techdebt.test.ts reminder.

## Problem There's no real reason we need to use node 16 in CI, and it's preventing us from using some features. Our tests run against vscode's own version of node, which is unrelated to the node we use to run scripts, etc. (See also: #5761 ) ## Solution update CI runners to node 18 instead of 16. We need to pass ` { shell: true } ` to avoid error on windows. see more here: https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2 ---  License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Justin M. Keyes <[email protected]>

Hweinstock added 3 commits October 9, 2024 16:39

move over to fs

bb7d90c

add type signature

823d6f7

switch to cpSYnc

54ae361

justinmk3 reviewed Oct 9, 2024

View reviewed changes

packages/amazonq/scripts/build/copyFiles.ts Show resolved Hide resolved

move into main function

f246422

Hweinstock added 8 commits October 10, 2024 11:01

add experimental node flag

19525fe

adjust flag, add techdebt test

1b5ff35

change flag

2fd7868

Merge branch 'master' into updateScripts

5256feb

fix typo

7ed0296

make the flags contained in bash file

21f1728

remove experimental flag

9c23639

try type assertions

5b72dd0

Hweinstock added 3 commits October 16, 2024 11:30

Merge branch 'master' into updateScripts

1bd5782

add dependency

7c5e40f

update package-lock

be2e88d

Hweinstock marked this pull request as ready for review October 16, 2024 15:50

Hweinstock requested a review from a team as a code owner October 16, 2024 15:50

justinmk3 reviewed Oct 16, 2024

View reviewed changes

justinmk3 merged commit 5cbe42b into aws:master Oct 16, 2024
31 of 34 checks passed

Hweinstock deleted the updateScripts branch October 16, 2024 21:51

This was referenced Oct 16, 2024

deps(types-node): avoid formal dependency on types-node22 #5801

Merged

deps(fs-extra): completely remove fs-extra dep. #5803

Merged

deps(ci node): update node in ci to 18.x [WIP] #5810

Merged

justinmk3 pushed a commit that referenced this pull request Oct 17, 2024

deps: drop fs-extra #5803

30f21dc

## Problem Follow up from #5761 (comment). ## Solution `npm uninstall --save '@types/fs-extra' 'fs-extra' -w packages/core/`

tverney pushed a commit to tverney/aws-toolkit-vscode that referenced this pull request Oct 21, 2024

deps: drop fs-extra aws#5803

9a6a097

## Problem Follow up from aws#5761 (comment). ## Solution `npm uninstall --save '@types/fs-extra' 'fs-extra' -w packages/core/`

	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'),

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

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

Uh oh!

Conversation

Hweinstock commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

Uh oh!

Hweinstock commented Oct 9, 2024

Uh oh!

justinmk3 commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hweinstock commented Oct 10, 2024

Uh oh!

justinmk3 commented Oct 11, 2024

Uh oh!

Hweinstock commented Oct 14, 2024

Uh oh!

justinmk3 commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hweinstock commented Oct 16, 2024

Uh oh!

justinmk3 Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

Hweinstock Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

justinmk3 Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

justinmk3 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

justinmk3 commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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

Hweinstock commented Oct 9, 2024 •

edited

Loading

justinmk3 commented Oct 10, 2024 •

edited

Loading

justinmk3 commented Oct 14, 2024 •

edited

Loading

justinmk3 commented Oct 16, 2024 •

edited

Loading