Skip to content

Upgrades TypeScript to 4.1, simplifies jest config#1230

Closed
tmarkley wants to merge 1 commit intoopensearch-project:mainfrom
tmarkley:nth-check
Closed

Upgrades TypeScript to 4.1, simplifies jest config#1230
tmarkley wants to merge 1 commit intoopensearch-project:mainfrom
tmarkley:nth-check

Conversation

@tmarkley
Copy link
Copy Markdown
Contributor

@tmarkley tmarkley commented Feb 10, 2022 •
edited
Loading

Description

Most of the PR is just adding parameters to Promises and changing references to the updated @osd/test/jest directory.

  • Addressing the nth-check CVE required bumping css-select, which is a dependency of cheerio. Bumping cheerio requires upgrading from TypeScript 4.0 to 4.1.
  • TypeScript 4.1 introduces a set of breaking changes. The main changes that impact Dashboards is that resolve's parameters are no longer optional in Promises, and that potentially undefined indexes must use the ! non-null assertion operator.
  • The upgrades to TypeScript and cheerio triggered some jest errors which prompted the upgrade to the enzyme dependencies.
  • Merges files under /src/test_utils and /src/dev/jest into the @osd/test package to simplify.
  • Fixes the naming of the @osd/eslint-config-opensearch-dashboards package.

Issues Resolved

Resolves #1081

Partially addresses #1187

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@tmarkley tmarkley added dependencies Pull requests that update a dependency file refactor Tech debt related tasks that need refactoring labels Feb 10, 2022
@tmarkley tmarkley requested a review from a team February 10, 2022 01:51
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@markov00
fix(heatmap): pick correct brush end value (opensearch-project#1230)
cb95a75 
fix opensearch-project#1229
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 31.1.0 [skip ci]
ad6591f 
# [31.1.0](elastic/elastic-charts@v31.0.0...v31.1.0) (2021-07-06)

### Bug Fixes

* **heatmap:** pick correct brush end value ([opensearch-project#1230](elastic/elastic-charts#1230)) ([cb95a75](elastic/elastic-charts@cb95a75)), closes [opensearch-project#1229](elastic/elastic-charts#1229)

### Features

* **a11y:** accessible goal and gauge chart ([#1174](elastic/elastic-charts#1174)) ([775dc98](elastic/elastic-charts@775dc98)), closes [opensearch-project#1160](elastic/elastic-charts#1160)
@tmarkley
Copy link
Copy Markdown
Contributor Author

tmarkley commented Feb 10, 2022

I'm taking a look at the error that the functional tests are throwing:

It looks like you called `mount()` without a global document being loaded.
    at new ReactWrapper (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/enzyme/src/ReactWrapper.js:103:13)
    at mount (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/enzyme/src/mount.js:10:10)
    at Object.<anonymous> (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-test/src/jest/utils/enzyme_helpers.tsx:46:19)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Module._compile (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Object.newLoader [as .js] (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Module.require (internal/modules/cjs/loader.js:974:19)
    at Module.Hook._require.Module.require (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/require-in-the-middle/index.js:80:39)
    at Module.Hook._require.Module.require (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/require-in-the-middle/index.js:80:39)
    at Module.Hook._require.Module.require (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/require-in-the-middle/index.js:80:39)
    at Module.Hook._require.Module.require (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/require-in-the-middle/index.js:80:39)
    at Module.Hook._require.Module.require (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/require-in-the-middle/index.js:80:39)
    at require (internal/modules/cjs/helpers.js:93:18)

@tmarkley tmarkley mentioned this pull request Feb 25, 2022
Merged
7 tasks
@tmarkley
Copy link
Copy Markdown
Contributor Author

tmarkley commented Mar 1, 2022 •
edited
Loading

Currently trying to fix the bootstrap errors related to istanbul-lib-coverage types. It may be easier to resolve this problem after #1300 and #1301 are merged.

ERROR [bootstrap] failed:
ERROR Error: Command failed with exit code 1: /opt/hostedtoolcache/node/14.18.2/x64/lib/node_modules/yarn/bin/yarn.js run osd:bootstrap
      error Command failed with exit code 1.
      $ node scripts/build --source-maps
       info Deleting old output
       info Starting babel and typescript
       info [babel     ] > babel src --config-file /home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-test/babel.config.js --out-dir /home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-test/target --extensions .ts,.js,.tsx --quiet --source-maps inline
       info [tsc       ] > tsc --declarationMap true
       info [babel     ] exited with 0 after a few seconds
       proc [tsc       ] ../../node_modules/@jest/reporters/build/generateEmptyCoverage.d.ts(8,30): error TS7016: Could not find a declaration file for module 'istanbul-lib-coverage'. '/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/@jest/reporters/node_modules/istanbul-lib-coverage/index.js' implicitly has an 'any' type.
       proc [tsc       ]   If the 'istanbul-lib-coverage' package actually exposes this module, consider sending a pull request to amend '[https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/istanbul-lib-coverage`](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/istanbul-lib-coverage%60)
      ERROR UNHANDLED ERROR
      ERROR Error: [tsc       ] exited with code 2
                at Object.createCliError (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-dev-utils/target/proc_runner/errors.js:35:19)
                at MapSubscriber.project (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-dev-utils/target/proc_runner/proc.js:100:28)
                at MapSubscriber._next (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/rxjs/internal/operators/map.js:49:35)
                at MapSubscriber.Subscriber.next (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/rxjs/internal/Subscriber.js:66:18)
                at TakeSubscriber._next (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/rxjs/internal/operators/take.js:54:30)
                at TakeSubscriber.Subscriber.next (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/rxjs/internal/Subscriber.js:66:18)
                at ChildProcess.handler (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/node_modules/rxjs/internal/observable/fromEvent.js:19:28)
                at ChildProcess.emit (events.js:412:35)
                at Process.ChildProcess._handle.onexit (internal/child_process.js:282:12)
      info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
          at makeError (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:25139:11)
          at handlePromise (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:24074:26)
          at processTicksAndRejections (internal/process/task_queues.js:95:5)
          at async /home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:9040:9
          at async scheduleItem (/home/runner/work/OpenSearch-Dashboards/OpenSearch-Dashboards/packages/osd-pm/dist/index.js:10927:9)

@kavilla
Copy link
Copy Markdown
Member

kavilla commented Mar 2, 2022 •
edited
Loading

Merges files under /src/test_utils and /src/dev/jest into the @osd/test package to simplify.

Would this reduce the delta changed in this PR but resolve the CVE if the merging happened in a separate PR?

@tmarkley
Copy link
Copy Markdown
Contributor Author

tmarkley commented Mar 2, 2022

Merges files under /src/test_utils and /src/dev/jest into the @osd/test package to simplify.

Would this reduce the delta changed in this PR but resolve the CVE if the merging happened in a separate PR?

Yeah, but I can't get the TypeScript upgrade to bootstrap even without those jest changes. There's a conflict with the jest dependencies so I'm going to wait for #1301 and then try again.

@tmarkley tmarkley force-pushed the nth-check branch 2 times, most recently from 3b5c51a to 5027633 Compare March 3, 2022 22:57
@tmarkley
Copy link
Copy Markdown
Contributor Author

tmarkley commented Mar 4, 2022

Currently trying to fix the bootstrap errors related to istanbul-lib-coverage types. It may be easier to resolve this problem after #1300 and #1301 are merged.

This has been resolved after merging those PRs and rebasing.

ashwin-pc
ashwin-pc previously approved these changes Mar 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread src/cli/serve/integration_tests/reload_logging_config.test.ts
]);

const message$ = Rx.fromEvent(child.stdout, 'data').pipe(
const message$ = Rx.fromEvent(child.stdout!, 'data').pipe(
Copy link
Copy Markdown
Member

@ashwin-pc ashwin-pc Mar 4, 2022

Choose a reason for hiding this comment

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

Nit: This is nullable. Given that its a test it should be okay though.

@tmarkley tmarkley force-pushed the nth-check branch 2 times, most recently from cdc3585 to 7c1155d Compare March 4, 2022 01:48
@tmarkley tmarkley force-pushed the nth-check branch 3 times, most recently from bf318d4 to 1f3399a Compare March 15, 2022 17:38
@tmarkley tmarkley added the cve Security vulnerabilities detected by Dependabot or Mend label Mar 16, 2022
@tmarkley tmarkley force-pushed the nth-check branch 2 times, most recently from e35cbfa to f44d550 Compare March 23, 2022 21:59
@kavilla
Copy link
Copy Markdown
Member

kavilla commented Mar 24, 2022

Did we want to put this PR in draft?

@tmarkley
Copy link
Copy Markdown
Contributor Author

tmarkley commented Mar 24, 2022

Did we want to put this PR in draft?

Good question. I'm still actively trying to fix the functional test error, but I guess I can put it in draft until that's fixed.

@tmarkley tmarkley marked this pull request as draft March 24, 2022 18:02
@kavilla
Copy link
Copy Markdown
Member

kavilla commented Apr 5, 2022

Looks like the CVE was addressed in this PR. For that reason and the conflicts, I will remove the 2.0.0.

@kavilla kavilla added v3.0.0 and removed v2.0.0 labels Apr 5, 2022
Upgrades TypeScript to 4.1, simplifies jest config
e15c939 
* Addressing the `nth-check` CVE requires bumping `css-select`, which
  is a dependency of `cheerio`. Bumping `cheerio` requires upgrading
  from TypeScript 4.0 to 4.1.
* TypeScript 4.1 introduces a set of [breaking changes](https://devblogs.microsoft.com/typescript/announcing-typescript-4-1/#breaking-changes).
  The main changes that impact Dashboards is that `resolve`'s parameters
  are no longer optional in `Promise`s, and that potentially undefined
  indexes must use the `!` non-null assertion operator.
* The upgrades to TypeScript and `cheerio` triggered some jest errors
  which prompted the upgrade to the `enzyme` dependencies.
* Merges files under `/src/test_utils` and `/src/dev/jest` into the
  `@osd/test` package to simplify.
* Fixes the naming of the `@osd/eslint-config-opensearch-dashboards`
  package.
* Fixes inconsistent plugin installation tests.

Resolves opensearch-project#1081

Signed-off-by: Tommy Markley <markleyt@amazon.com>
@kavilla
Copy link
Copy Markdown
Member

kavilla commented Apr 25, 2022

Verify impact to plugins.

@seanneumann
Copy link
Copy Markdown
Contributor

seanneumann commented Sep 26, 2022

Closing. We are not doing a TypeScript upgrade now as the refactor is too much for the benefit. We can revisit this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashwin-pc ashwin-pc ashwin-pc left review comments

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@ananzh ananzh Awaiting requested review from ananzh ananzh is a code owner

@kavilla kavilla Awaiting requested review from kavilla kavilla is a code owner

@abbyhu2000 abbyhu2000 Awaiting requested review from abbyhu2000 abbyhu2000 will be requested when the pull request is marked ready for review abbyhu2000 is a code owner

@zengyan-amazon zengyan-amazon Awaiting requested review from zengyan-amazon zengyan-amazon will be requested when the pull request is marked ready for review zengyan-amazon is a code owner

@zhongnansu zhongnansu Awaiting requested review from zhongnansu zhongnansu will be requested when the pull request is marked ready for review zhongnansu is a code owner

@ZilongX ZilongX Awaiting requested review from ZilongX ZilongX will be requested when the pull request is marked ready for review ZilongX is a code owner

@Flyingliuhub Flyingliuhub Awaiting requested review from Flyingliuhub Flyingliuhub will be requested when the pull request is marked ready for review Flyingliuhub is a code owner

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn will be requested when the pull request is marked ready for review bandinib-amzn is a code owner

@SuZhou-Joe SuZhou-Joe Awaiting requested review from SuZhou-Joe SuZhou-Joe will be requested when the pull request is marked ready for review SuZhou-Joe is a code owner

@ruanyl ruanyl Awaiting requested review from ruanyl ruanyl will be requested when the pull request is marked ready for review ruanyl is a code owner

@xinruiba xinruiba Awaiting requested review from xinruiba xinruiba will be requested when the pull request is marked ready for review xinruiba is a code owner

@zhyuanqi zhyuanqi Awaiting requested review from zhyuanqi zhyuanqi will be requested when the pull request is marked ready for review zhyuanqi is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric will be requested when the pull request is marked ready for review mengweieric is a code owner

@LDrago27 LDrago27 Awaiting requested review from LDrago27 LDrago27 will be requested when the pull request is marked ready for review LDrago27 is a code owner

@virajsanghvi virajsanghvi Awaiting requested review from virajsanghvi virajsanghvi will be requested when the pull request is marked ready for review virajsanghvi is a code owner

@sejli sejli Awaiting requested review from sejli sejli will be requested when the pull request is marked ready for review sejli is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 will be requested when the pull request is marked ready for review joshuali925 is a code owner

@huyaboo huyaboo Awaiting requested review from huyaboo huyaboo will be requested when the pull request is marked ready for review huyaboo is a code owner

@angle943 angle943 Awaiting requested review from angle943 angle943 will be requested when the pull request is marked ready for review angle943 is a code owner

@Maosaic Maosaic Awaiting requested review from Maosaic Maosaic will be requested when the pull request is marked ready for review Maosaic is a code owner

@wanglam wanglam Awaiting requested review from wanglam wanglam will be requested when the pull request is marked ready for review wanglam is a code owner

@d-buckner d-buckner Awaiting requested review from d-buckner d-buckner will be requested when the pull request is marked ready for review d-buckner is a code owner

@ps48 ps48 Awaiting requested review from ps48 ps48 will be requested when the pull request is marked ready for review ps48 is a code owner

@TackAdam TackAdam Awaiting requested review from TackAdam TackAdam will be requested when the pull request is marked ready for review TackAdam is a code owner

@ruchidh ruchidh Awaiting requested review from ruchidh ruchidh will be requested when the pull request is marked ready for review ruchidh is a code owner

@sumukhswamy sumukhswamy Awaiting requested review from sumukhswamy sumukhswamy will be requested when the pull request is marked ready for review sumukhswamy is a code owner

@paulstn paulstn Awaiting requested review from paulstn paulstn will be requested when the pull request is marked ready for review paulstn is a code owner

@Qxisylolo Qxisylolo Awaiting requested review from Qxisylolo Qxisylolo will be requested when the pull request is marked ready for review Qxisylolo is a code owner

@yubonluo yubonluo Awaiting requested review from yubonluo yubonluo will be requested when the pull request is marked ready for review yubonluo is a code owner

Assignees

No one assigned

Labels

cve Security vulnerabilities detected by Dependabot or Mend dependencies Pull requests that update a dependency file refactor Tech debt related tasks that need refactoring v3.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CVE-2021-3803 (High) detected in nth-check-1.0.2.tgz

4 participants

@tmarkley @kavilla @seanneumann @ashwin-pc