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

Fix TypeScript use; add to CI #2884

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Fix TypeScript use; add to CI #2884

merged 3 commits into from
Feb 1, 2024

Conversation

adekbadek
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Some TS was added in this project, but a CI step to keep it checked was not added. Since we don't compile TS, but only verify it's validity, the TS code was invalid but we would not know (until verifying locally, e.g. via IDE). This PR adds a CI step to verify TS, fixes* the existing files, and converts a couple of files to TS.

How to test the changes in this Pull Request:

  1. Observe the check-typescript CI job passing 👇
  2. Review code changes – there should be no functional changes, just pure refactor and added checks to satisfy TS

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@adekbadek adekbadek added the [Status] Needs Review The issue or pull request needs to be reviewed label Jan 26, 2024
@adekbadek adekbadek requested a review from a team as a code owner January 26, 2024 15:44
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Not seeing any functional issues here, but running the lint:js The warnings are expected, right?

/Users/dkoo/Repositories/newspack-docker/repos/newspack-plugin/assets/components/src/button/index.tsx
  44:2  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment

/Users/dkoo/Repositories/newspack-docker/repos/newspack-plugin/assets/wizards/engagement/components/prerequisite.tsx
  83:12  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment

/Users/dkoo/Repositories/newspack-docker/repos/newspack-plugin/assets/wizards/engagement/components/prompt.tsx
  104:5   error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  200:14  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  204:15  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  206:16  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  208:17  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  213:15  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  215:16  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  231:11  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  234:12  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  246:11  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  254:11  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  257:12  error  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment

/Users/dkoo/Repositories/newspack-docker/repos/newspack-plugin/assets/wizards/readerRevenue/views/donation/index.tsx
   91:68  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any
  257:55  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 16 problems (14 errors, 2 warnings)

Approved pending a merge conflict resolution.

# Conflicts:
#	package-lock.json
#	package.json
@adekbadek
Copy link
Member Author

The warnings are expected, right?

Yes, these are to remind us that having these hacks is not ideal and things should be typed properly. The "alters compilation errors" does not apply here because we don't compile TS.

@adekbadek adekbadek merged commit 6f5e7a6 into master Feb 1, 2024
6 checks passed
@adekbadek adekbadek deleted the fix/typescript branch February 1, 2024 09:19
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 8, 2024
# [3.1.0-alpha.1](v3.0.3...v3.1.0-alpha.1) (2024-02-08)

### Bug Fixes

* **engagement-wizard:** handle error when retrieving subscription lists ([e85c108](e85c108))
* **ras:** only sync spend total and last payment amounts for completed orders ([#2886](#2886)) ([68aaf39](68aaf39))
* redirect to origin from magic link ([9f41947](9f41947))
* typescript errors ([dc27973](dc27973))
* TypeScript usage; add to CI ([#2884](#2884)) ([6f5e7a6](6f5e7a6))
* update newsletter scroll appearance in Sign Up modal ([#2897](#2897)) ([496723a](496723a))

### Features

* **ci:** add epic/* release workflow and rename `master` to `trunk` ([#2895](#2895)) ([ea02075](ea02075)), closes [#2897](#2897) [#2886](#2886)
* **reader-revenue:** make NYP and Stripe Gateway optional ([#2866](#2866)) ([fcfa88c](fcfa88c))
* remove new tab default on image credits ([#2880](#2880)) ([3c996b1](3c996b1))
* **wc:** override cart, checkout, and my-account page templates ([#2893](#2893)) ([68b1836](68b1836))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 20, 2024
# [3.1.0](v3.0.5...v3.1.0) (2024-02-20)

### Bug Fixes

* add frequency tab options for donations, even when tiers are disabled ([#2930](#2930)) ([cb7eb7b](cb7eb7b))
* **categories:** fix pager urls ([#2913](#2913)) ([bb7e534](bb7e534))
* **categories:** fix pager urls ([#2913](#2913)) ([c851bb6](c851bb6))
* **engagement-wizard:** handle error when retrieving subscription lists ([e85c108](e85c108))
* **ras:** only sync spend total and last payment amounts for completed orders ([#2886](#2886)) ([68aaf39](68aaf39))
* redirect to origin from magic link ([9f41947](9f41947))
* typescript errors ([dc27973](dc27973))
* TypeScript usage; add to CI ([#2884](#2884)) ([6f5e7a6](6f5e7a6))
* update newsletter scroll appearance in Sign Up modal ([#2897](#2897)) ([496723a](496723a))
* update path to wide template file ([#2918](#2918)) ([fdd6b69](fdd6b69))

### Features

* **ci:** add epic/* release workflow and rename `master` to `trunk` ([#2895](#2895)) ([ea02075](ea02075)), closes [#2897](#2897) [#2886](#2886)
* **reader-revenue:** make NYP and Stripe Gateway optional ([#2866](#2866)) ([fcfa88c](fcfa88c))
* remove new tab default on image credits ([#2880](#2880)) ([3c996b1](3c996b1))
* **wc:** override cart, checkout, and my-account page templates ([#2893](#2893)) ([68b1836](68b1836))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants