-
Notifications
You must be signed in to change notification settings - Fork 48
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
chore/update-typescript-eslint-and-module-resolution #624
base: development
Are you sure you want to change the base?
Conversation
207ffc4
to
13847f6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #624 +/- ##
============================================
Coverage 54.52% 54.53%
============================================
Files 226 226
Lines 5199 5200 +1
Branches 800 800
============================================
+ Hits 2835 2836 +1
Misses 2127 2127
Partials 237 237 ☔ View full report in Codecov by Sentry. |
This was previously implicit and depended on a the root build. This clashes with my attempts to update the typescript version, so I am scoping the e2e tests to their own build.
02960b6
to
123d3c6
Compare
"target": "es2022" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */, | ||
"lib": ["es2023"], |
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.
Are these compatible with recent versions of vscode?
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 is based on:
- VSCode is on Node 20
- The tsconfig for working with Node 20 suggests the target and lib selection above: https://www.npmjs.com/package/@tsconfig/node20
However, this was only tentative, it should all be reviewed once the all the parts allow a run through of Slang with WASM.
There is a question about whether we should target Node 18 rather than Node 20 so that we are keeping to the Hardhat node version support promise. This wouldn't affect the VSCode library but you can run the language server independently.
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.
I think that if it works with vscode is enough. We don't need to keep the same support, as vscode updates on a different cadence.
e9043b2
to
5f6c757
Compare
This brings coc inline with the eslint config pattern found for other packages in the repo. It showed up as an issue when attempting to update the typescript version.
The language server was 2 years out of date. To maximize ease of integration with typescript@5 we are updating to the latest version.
The latest version of lodash types is been taken to better work with typescript@5.
Eslint and its plugins are 2 years out of date. They have been brought up to the same version used in the Hardhat repo. We initially tried to jump straight to `eslint@10` but backed out with the config format changes. We will look at this again with the repo modernization efforts.
Node 20 is now the version used in VSCode. The types have been updated to reflect the new node versions.
5f6c757
to
471a696
Compare
To package the extension we use a bundler, esbuild needs to be updated to match the new version of typescript.
We need better module resolution support as we deal with Slang's ESM packages, specifically the loading of TS types from commonjs.
To support pulling in more modern ts libraries like Slang@18, we want to swap to `node16` module resolution. This is not ESM, but the modern version of commonjs. It lets us use TS types from Slang from the VSCode extension's commonjs packages.
This is a hack that we should accept temporarily. A library that the language server node library uses fails more refined typechecking in typescript@5. For the moment checking types in dependent packages is skipped.
The update to eslint revealed this equality error.
New rules are affecting our naming conventions. Rather than disable the rule I have scattered disable lines. This is a tradeoff, but we expect a revamp of the eslint rules based on HH3 in the near future.
471a696
to
91c7944
Compare
"coc.nvim": "^0.0.80", | ||
"esbuild": "^0.16.0", | ||
"eslint": "^7.23.0" | ||
"coc.nvim": "^0.0.80" |
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.
Won't our build/lint scripts fail if we remove those dev dependencies?
@kanej Besides the 1 question I left, I'm seeing the "package" step on the CI is failing. I don't see any changes in the bundle script so it's interesting. Should I look into it ? |
5e4f1bf
to
91c7944
Compare
Slang@19
supports being used from commonjs (which the vscode extension is). However, to get access to the exported types we require updating the module resolution we are using tonode16
for typescript builds.This module resolution option requires we update the version of typescript being used.
This PR:
[email protected]
, including[email protected]