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

chore: update typescript and typescript/eslint plugin to v4 #37

Merged
merged 19 commits into from
Sep 16, 2020

Conversation

mohanraj-r
Copy link
Contributor

@mohanraj-r mohanraj-r commented Sep 8, 2020

Fixes #27

Chores 🧹

Refactor ♻

  • Fix code to take care of the new errors from the upgraded versions
    • Silencing rules only as last resort esp. in non application code (test, config etc)
  • Fix how default options are processed for formatter
  • Fix chained npm cmds to depend on previous exit code
  • Reuse common typescript config for eslint

Post merge

@mohanraj-r mohanraj-r requested a review from a team as a code owner September 8, 2020 22:46
@mohanraj-r mohanraj-r self-assigned this Sep 8, 2020
@mohanraj-r
Copy link
Contributor Author

@trevor-bliss Can you please give this a glance

@@ -11,6 +11,8 @@ import { htmlFileWithA11yIssues, htmlFileWithNoA11yIssues } from '@sa11y/test-ut
import { A11yError } from '@sa11y/format';
import { axeRuntimeExceptionMsgPrefix } from '@sa11y/common';

// TODO (chore): Raise issue with WebdriverIO - 'sync' missing 'default' in ts def
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-var-requires
const sync = require('@wdio/sync').default;

Choose a reason for hiding this comment

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

What about this?

import sync from '@wdio/sync';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that. It results in the tests being skipped with wdio runner reporting that 1 suite passed without running any tests weirdly and also the following error.

TS1259: Module '"@wdio/sync"' can only be default-imported using the 'esModuleInterop' flag  webdriverio.d.ts(46, 5): This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

I do have the "esModuleInterop": true, in tsconfig.

Also tried import sync = require('@wdio/sync'); as suggested in the TypeScript: Handbook - Modules

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

Even though typescript is happy with that, the tests continue to be skipped.

Think this could be related to the fact that ts-node is used by wdio.conf.js and it might be interpreting the tsconfig differently? I tried reusing babel (which is used by jest) instead of ts-node but couldn't get it working (webdriverio/webdriverio#5588)

Please let me know if you have any suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO to reflect this for now

Choose a reason for hiding this comment

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

Hm weird... Yeah that's fine.

trevor-bliss
trevor-bliss previously approved these changes Sep 15, 2020
Copy link

@trevor-bliss trevor-bliss left a comment

Choose a reason for hiding this comment

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

Suggestion for importing the sync API from wdio, rest looks good.

Copy link
Contributor Author

@mohanraj-r mohanraj-r left a comment

Choose a reason for hiding this comment

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

Thanks for the review @trevor-bliss !

@@ -11,6 +11,8 @@ import { htmlFileWithA11yIssues, htmlFileWithNoA11yIssues } from '@sa11y/test-ut
import { A11yError } from '@sa11y/format';
import { axeRuntimeExceptionMsgPrefix } from '@sa11y/common';

// TODO (chore): Raise issue with WebdriverIO - 'sync' missing 'default' in ts def
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-var-requires
const sync = require('@wdio/sync').default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that. It results in the tests being skipped with wdio runner reporting that 1 suite passed without running any tests weirdly and also the following error.

TS1259: Module '"@wdio/sync"' can only be default-imported using the 'esModuleInterop' flag  webdriverio.d.ts(46, 5): This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

I do have the "esModuleInterop": true, in tsconfig.

Also tried import sync = require('@wdio/sync'); as suggested in the TypeScript: Handbook - Modules

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

Even though typescript is happy with that, the tests continue to be skipped.

Think this could be related to the fact that ts-node is used by wdio.conf.js and it might be interpreting the tsconfig differently? I tried reusing babel (which is used by jest) instead of ts-node but couldn't get it working (webdriverio/webdriverio#5588)

Please let me know if you have any suggestions.

@@ -11,6 +11,8 @@ import { htmlFileWithA11yIssues, htmlFileWithNoA11yIssues } from '@sa11y/test-ut
import { A11yError } from '@sa11y/format';
import { axeRuntimeExceptionMsgPrefix } from '@sa11y/common';

// TODO (chore): Raise issue with WebdriverIO - 'sync' missing 'default' in ts def
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-var-requires
const sync = require('@wdio/sync').default;

Choose a reason for hiding this comment

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

Hm weird... Yeah that's fine.

@mohanraj-r mohanraj-r merged commit bc3b2c8 into salesforce:master Sep 16, 2020
@mohanraj-r mohanraj-r deleted the update_typescript_4 branch September 16, 2020 19:51
@mohanraj-r mohanraj-r restored the update_typescript_4 branch September 16, 2020 19:52
@mohanraj-r mohanraj-r deleted the update_typescript_4 branch September 16, 2020 19:54
mohanraj-r added a commit to mohanraj-r/sa11y that referenced this pull request Sep 17, 2020
after merging upgrade to typescript 4 (salesforce#37) into master
mohanraj-r added a commit to mohanraj-r/sa11y that referenced this pull request Sep 17, 2020
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.

test(lint): upgrade to typescript-eslint v3.x, typescript v4.x and fix newly identified issues
2 participants