Skip to content
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

typecheck tests and bump typescript-eslint #2674

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 30, 2024

evergreen

Description

The typescript-eslint dep was behind so I tried to bump it. The major version breaking change required linted files to be included in a tsconfig. Files in demo, scripts and test were linted but not typechecked, so I added those to the tsconfigs. Then I did a bunch of work to get them to green.

I tried to commit incrementally and mostly stick to one package per commit. There are no stacked changes so you could review by file in HEAD and not miss anything.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

CI

Compatibility Considerations

Some slight changes to type interfaces but no breaking runtime changes

Upgrade Considerations

Update NEWS.md for user-facing changes.
? necessary?

hack around,

/opt/agoric/endo/packages/bundle-source/demo/reexport-fortune-ts.js
  0:0  error  Parsing error: /opt/agoric/endo/packages/bundle-source/demo/reexport-fortune-ts.js was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject

/opt/agoric/endo/packages/bundle-source/demo/reexport-meaning-js.js
  0:0  error  Parsing error: /opt/agoric/endo/packages/bundle-source/demo/reexport-meaning-js.js was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject
Work around:

dist/types.d.cts:558:16 - error TS2300: Duplicate identifier 'Compartment'.

558   export class Compartment {
                   ~~~~~~~~~~~

  types.d.ts:559:16
    559   export class Compartment {
                       ~~~~~~~~~~~
    'Compartment' was also declared here.
Without this building exported.js fails because it would overwrite the exported.d.ts in scm
@turadg turadg mentioned this pull request Jan 3, 2025
Copy link
Contributor

@boneskull boneskull Jan 15, 2025

Choose a reason for hiding this comment

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

maybe exclude this dir from typechecking? same with other frequent @ts-nocheck directives

@@ -9,6 +9,7 @@ import bundleSource from '../src/index.js';
* @param {string} entry
* @param {Options} options
*/
// @ts-expect-error 'Options' could be instantiated with a different subtype of constraint 'Partial<any>'.
const generate = async (entry, options = {}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const generate = async (entry, options = {}) => {
const generate = async (entry, options) => {

This should suppress the error, but then you'll need to supply an empty object somewhere below

@@ -11,6 +11,7 @@ test('no-transforms applies no transforms', async t => {
const entryPath = url.fileURLToPath(
new URL(`../demo/circular/a.js`, import.meta.url),
);
// @ts-expect-error Property 'endoZipBase64' does not exist on type '{ moduleFormat: "endoScript"; source: string; } | { moduleFormat: "endoZipBase64"; endoZipBase64: string; endoZipBase64Sha512: string; } | { moduleFormat: "nestedEvaluate"; source: string; sourceMap: string; } | { ...; }'.
Copy link
Contributor

@boneskull boneskull Jan 15, 2025

Choose a reason for hiding this comment

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

looks like we'll need to use @overload to define bundleSource

@@ -11,6 +11,7 @@ test('test objectMetaMap', async t => {
? undefined
: {
...desc,
// @ts-expect-error desc.value possibly undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably throw a t.assert.ok(desc.value) before this statement

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

many comments and questions but nothing blockworthy

@@ -19,6 +19,7 @@ export const parseJsonp = (bytes, _specifier, _location, _packageLocation) => {
const compartment = new Compartment({
__options__: true,
globals: harden({
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we should enable the rule that requires an explanation for use of @ts-expect-error

@@ -11,6 +11,7 @@ const fixture = new URL(
import.meta.url,
).toString();

// @ts-expect-error XXX Node interface munging
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted this to actually work. 😄

  • @boneskull investigate why this is causing an error

@@ -88,7 +89,7 @@ export function scaffold(
assertFixture,
fixtureAssertionCount,
{
onError,
onError = /** @type {(t, {error, title})} */ (undefined),
Copy link
Contributor

Choose a reason for hiding this comment

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

imo this should be a @ts-expect-error instead of this thing that is plainly untrue 😄

@@ -37,7 +37,8 @@ export const make = () => {
};

/**
* @type {NameHub['write']}
* @param {string[]} petNamePath
Copy link
Contributor

Choose a reason for hiding this comment

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

what was wrong with NameHub? do we still need that type?

'@typescript-eslint/restrict-plus-operands': 'warn',
// XXX override for RESM concession below
'@endo/no-optional-chaining': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, I thought this rule was actually needed?

@@ -122,7 +122,9 @@ test('packages in-the-wild', t => {

function testContent6() {
const list = [];
list.push = function newPush() {};
list.push = function newPush(...args) {
return args.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -16,7 +16,9 @@ test('lockdown start Date is powerful', t => {
});

test('lockdown Date.prototype.constructor is powerless', t => {
const SharedDate = Date.prototype.constructor;
const SharedDate = /** @type {DateConstructor} */ (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Date not Date.prototype.constructor? cc @erights

@@ -2,12 +2,15 @@
"extends": "../../tsconfig.eslint-base.json",
"compilerOptions": {
"allowJs": true,
"skipLibCheck": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

skipLibCheck: false probably also why linting is slow

@@ -135,10 +135,12 @@ export type ModuleMapHook = (
moduleSpecifier: string,
) => ModuleDescriptor | undefined;
export type ImportHook = (moduleSpecifier: string) => Promise<ModuleDescriptor>;
export type ImportNowHook = (moduleSpecifier: string) => ModuleDescriptor;
export type ImportNowHook = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? Can it return undefined?

@@ -19,6 +19,7 @@ test('zip round trip', async t => {

const reader = new ZipReader(writer.snapshot());
const text = textDecoder.decode(reader.read('hello/hello.txt'));
// @ts-expect-error undefined if file not found
Copy link
Contributor

Choose a reason for hiding this comment

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

would probably assert the result is truthy instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants