You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hainenber (via GitHub)" <gi...@apache.org> on 2025/01/17 15:46:25 UTC
[PR] chore: replace Lodash usage with native JS implementation [superset]
hainenber opened a new pull request, #31907:
URL: https://github.com/apache/superset/pull/31907
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
chore: replace Lodash usage with native JS implementation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
I noticed these replaceable `lodash` usage in the codebase when refactoring Enzyme-based unit tests to RTL and made a corresponding PR to address them afterwards.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
All CI checks should pass
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: replace Lodash usage with native JS implementation [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #31907:
URL: https://github.com/apache/superset/pull/31907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: replace Lodash usage with native JS implementation [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #31907:
URL: https://github.com/apache/superset/pull/31907#discussion_r1920425029
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -280,7 +280,7 @@ export const SaveDatasetModal = ({
// Remove the special filters entry from the templateParams
// before saving the dataset.
let templateParams;
- if (isString(datasource?.templateParams)) {
+ if (typeof datasource?.templateParams === 'string') {
const p = JSON.parse(datasource.templateParams);
Review Comment:
Not my change though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: replace Lodash usage with native JS implementation [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #31907:
URL: https://github.com/apache/superset/pull/31907#discussion_r1920391638
##########
superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx:
##########
@@ -280,7 +280,7 @@ export const SaveDatasetModal = ({
// Remove the special filters entry from the templateParams
// before saving the dataset.
let templateParams;
- if (isString(datasource?.templateParams)) {
+ if (typeof datasource?.templateParams === 'string') {
const p = JSON.parse(datasource.templateParams);
Review Comment:
### Unhandled JSON Parse Error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code doesn't handle JSON.parse errors which could occur if templateParams contains invalid JSON string.
###### Why this matters
If templateParams contains malformed JSON, the application will crash when trying to parse it, interrupting the save dataset operation.
###### Suggested change ∙ *Feature Preview*
Add try-catch block to handle potential JSON parsing errors:
```typescript
let templateParams;
if (typeof datasource?.templateParams === 'string') {
try {
const p = JSON.parse(datasource.templateParams);
/* eslint-disable-next-line no-underscore-dangle */
if (p._filters) {
/* eslint-disable-next-line no-underscore-dangle */
delete p._filters;
// eslint-disable-next-line no-param-reassign
templateParams = JSON.stringify(p);
}
} catch (error) {
console.error('Error parsing templateParams:', error);
templateParams = datasource.templateParams;
}
}
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:5a90b683-bc70-409a-800e-f02e735d51a8 -->
##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx:
##########
@@ -76,7 +75,7 @@ export function CustomFrame(props: FrameComponentProps) {
value: string | number,
) {
// only positive values in grainValue controls
- if (isInteger(value) && value > 0) {
+ if (typeof value === 'number' && Number.isInteger(value) && value > 0) {
Review Comment:
### Redundant Type Checking <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple type checks are being performed unnecessarily in sequence, with Number.isInteger() already implying the value is a number.
###### Why this matters
The redundant type checking adds unnecessary computation overhead, especially in frequently executed code paths.
###### Suggested change ∙ *Feature Preview*
Simplify the condition to only use Number.isInteger() which inherently checks for number type:
```typescript
if (Number.isInteger(value) && value > 0)
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.
<!--- korbi internal id:06c0e3e9-65b6-47c0-b5e8-4a0e43ffba4a -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] chore: replace Lodash usage with native JS implementation [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #31907:
URL: https://github.com/apache/superset/pull/31907#discussion_r1920426507
##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/CustomFrame.tsx:
##########
@@ -76,7 +75,7 @@ export function CustomFrame(props: FrameComponentProps) {
value: string | number,
) {
// only positive values in grainValue controls
- if (isInteger(value) && value > 0) {
+ if (typeof value === 'number' && Number.isInteger(value) && value > 0) {
Review Comment:
Without the typecheck in the beginning, there will be a type error
<img width="939" alt="image" src="https://github.com/user-attachments/assets/a39f97e1-6ab0-4c29-9bf7-2a46ec9df372" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org