Skip to content

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

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 6 commits into from
Oct 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5ea0cfd
implement any workaround
Hweinstock Oct 16, 2024
f6d8b09
add techdebt test
Hweinstock Oct 16, 2024
8a056c4
Update packages/core/src/test/techdebt.test.ts
Hweinstock Oct 16, 2024
e4f647f
try node 18 types as dep
Hweinstock Oct 16, 2024
53c87f4
Merge branch 'removeNodeTypes' of github.com:Hweinstock/aws-toolkit-v…
Hweinstock Oct 16, 2024
728a145
update message in techdebt
Hweinstock Oct 17, 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
16 changes: 8 additions & 8 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.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"webpack-merge": "^5.10.0"
},
"dependencies": {
"@types/node": "^22.7.5",
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.

Does it work if this is 18 instead of 22?

I'm confused about the relationship of the copyFiles.ts scripts vs the packages/x/package.json declarations.

As an alternative to fs as any, maybe we could use the techdebt.test.ts reminder to remind us to remove this top-level dependency when the getMinNodejsVersion result is new enough?

Copy link
Contributor Author

@Hweinstock Hweinstock Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like that works as well, I can update the techdebt test to match. I thought avoiding the dependency was more important than the any workaround. But I guess the version bump isn't something to worry about? Are there any other tradeoffs to consider?

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.

I thought avoiding the dependency was more important than the any workaround.

Since this PR makes the total cost clearer (i.e.: small :), I'm having doubts. And also, since the risk is covered by techdebt.test.ts in either case, perhaps it's better to have type checking.

Either way is fine IMO.

Copy link
Contributor Author

@Hweinstock Hweinstock Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'll keep the dependency then. Since its just a version bump (to an old stable version) and not a whole new thing, the cost seems very low. The any code doesn't read well, and could cause confusion to someone without the necessary context.

"@types/node": "^18.19.55",
"vscode-nls": "^5.2.0",
"vscode-nls-dev": "^4.0.4"
}
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/test/techdebt.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ describe('tech debt', function () {
semver.lt(minNodejs, '18.0.0'),
'with node16+, we can now use AbortController to cancel Node things (child processes, HTTP requests, etc.)'
)
// This is relevant for the use of `fs.cpSync` in the copyFiles scripts.
assert.ok(semver.lt(minNodejs, '18.0.0'), 'with node18+, we can remove the dependency on @types/node@18')
})

it('remove separate sessions login edge cases', async function () {
Expand Down
Loading