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(deps): bump typescript to 4.8.4 #24272

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

jansule
Copy link
Contributor

@jansule jansule commented Jun 2, 2023

SUMMARY

This bumps typescript from 4.5.4 to 4.8.4. @typescript-eslint/eslint-plugin and @typescript-eslint/parser were upgraded to 5.42.1 to match the typescript version.

New TSERRORs were fixed where possible. Unfortunately, I had to add a few @ts-ignores here and there. Maybe someone with a deeper understanding of the code base is able to fix these errors properly.

I decided to bump to typescript 4.8.4 as work related to #21758 introduced an upgrade to this version anyways. In consultation with @villebro we decided to create a dedicated PR for this.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Simply run the tests and build the application to see if any TSERRORs occur.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

Copy link
Member

@villebro villebro 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 bump! A few small comments, but looks good in general 👍

@@ -24,16 +24,16 @@ export default function extent<T = number | string | Date | undefined | null>(
// eslint-disable-next-line no-restricted-syntax
for (const value of values) {
if (value !== null) {
if (min === undefined) {
if (min === undefined || min === null) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could use _.isNil here?

let _ = require('lodash');
> _.isNil(undefined)
true
> _.isNil(null)
true
> _.isNil(0)
false
> _.isNil('')
false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in cbcc0ee

if (min > value) {
min = value;
}
if (max !== undefined) {
if (max !== undefined && max !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

Suggested change
if (max !== undefined && max !== null) {
if (!isNil(max)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in cbcc0ee

@@ -158,6 +158,7 @@ class WordCloud extends React.PureComponent<
update() {
const { data, encoding } = this.props;

// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

not a huge deal, but I'd be curious to understand what the error was here, do you have it handy?

Copy link
Contributor Author

@jansule jansule Jun 2, 2023

Choose a reason for hiding this comment

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

Sure, it's

Argument of type 'Partial<DeriveEncoding<WordCloudEncodingConfig>>' is not assignable to parameter of type 'never'

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just noticed it's still a class component. Let's revisit this some day when it's migrated to a functional component.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (64ba579) 67.22% compared to head (39706d4) 67.22%.

Files Patch % Lines
...end/plugins/plugin-chart-table/src/utils/extent.ts 0.00% 3 Missing ⚠️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #24272      +/-   ##
==========================================
- Coverage   67.22%   67.22%   -0.01%     
==========================================
  Files        1895     1895              
  Lines       74211    74212       +1     
  Branches     8245     8246       +1     
==========================================
  Hits        49891    49891              
- Misses      22248    22249       +1     
  Partials     2072     2072              
Flag Coverage Δ
javascript 56.98% <37.50%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jansule jansule force-pushed the typescript-upgrade branch from cbcc0ee to 71e2886 Compare June 2, 2023 12:08
@EugeneTorap
Copy link
Contributor

Hey @jansule! Can you merge master to your branch?

@jansule
Copy link
Contributor Author

jansule commented Aug 15, 2023

@EugeneTorap Sorry for the late reply. I will try to find time this week to rebase against current master.

@jansule jansule force-pushed the typescript-upgrade branch from 71e2886 to cf1d55a Compare August 16, 2023 13:56
@jansule
Copy link
Contributor Author

jansule commented Aug 16, 2023

@EugeneTorap just rebased against current master. Locally, the build succeeded.

@EugeneTorap
Copy link
Contributor

Hey @michael-s-molina! Can you review this PR?

@villebro
Copy link
Member

@jansule sorry for missing this PR - would you be able to rebase this so we can get it merged?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, unless the rebase turns up some new issues

@jansule jansule force-pushed the typescript-upgrade branch from cf1d55a to e6fa6b9 Compare December 4, 2023 15:14
@jansule
Copy link
Contributor Author

jansule commented Dec 4, 2023

@villebro I just rebased against current master. Locally the build is passing (docker build --target lean .). I also clicked very briefly through the application and did not encounter issues.

@rusackas
Copy link
Member

rusackas commented Dec 7, 2023

Running CI 🤞

@jansule
Copy link
Contributor Author

jansule commented Dec 14, 2023

I'm not sure how to interpret the failing check https://github.com/apache/superset/actions/runs/7088616773/job/19419142631?pr=24272:

────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                      
    Running:  explore/visualizations/sankey.test.js                                         (31 of 53)
    Estimated: 18 seconds
  
  
    Visualization > Sankey
      ✓ should work (3946ms)
      ✓ should work with filter (2418ms)
  Error: The operation was canceled.

I would be grateful, if somebody could give me hints here.

@jansule jansule force-pushed the typescript-upgrade branch from e6fa6b9 to 39706d4 Compare January 29, 2024 15:22
@rusackas rusackas merged commit 29582e8 into apache:master Jan 29, 2024
32 checks passed
eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024
@marcjansen marcjansen deleted the typescript-upgrade branch June 4, 2024 14:47
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 4.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants