Skip to content

Conversation

@shulaoda
Copy link
Member

closes #6750

According to eslint-plugin-react, it seems that we should also search for // @jsx pragma in the source code.

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 23, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-linter Area - Linter C-bug Category - Bug labels Oct 23, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 23, 2024

CodSpeed Performance Report

Merging #6819 will not alter performance

Comparing shulaoda:fix/linter-iframe-missing-sandbox (1b2e891) with main (a148023)

Summary

✅ 30 untouched benchmarks

Comment on lines 18 to 21
expr => expr.is_specific_member_access(
ctx.settings().react.pragma.as_ref().map_or("React", |pragma| pragma.as_str()),
"createElement",
),
Copy link
Contributor

Choose a reason for hiding this comment

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

There are also some false positive:

const createElement = document.createElement
createElement('iframe')

const React = myReact
React.createElement('iframe')

Can we port isDestructuredFromPragmaImport here? we can ignore the require part for this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

This might be a bit complicated, so let's focus on resolving the issue first, and we can improve the related code afterwards.

Copy link
Contributor

@mysteryven mysteryven Oct 24, 2024

Choose a reason for hiding this comment

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

Or not add ctx.settings().react.pragma for this issue:

const PRAGMA: &str = "React";

@shulaoda shulaoda marked this pull request as draft October 23, 2024 13:21
@shulaoda shulaoda force-pushed the fix/linter-iframe-missing-sandbox branch from 40ba4e6 to cc15346 Compare October 24, 2024 12:31
@github-actions github-actions bot added the A-cli Area - CLI label Oct 24, 2024
@shulaoda shulaoda marked this pull request as ready for review October 24, 2024 12:59
@shulaoda shulaoda requested a review from mysteryven October 24, 2024 12:59
"rootDir": []
},
"react": {
"pragma": "React",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep same with ESLint config file?

/// This configuration is aligned with ESLint v8's configuration schema (`eslintrc.json`).

Copy link
Member Author

Choose a reason for hiding this comment

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

I keep. Pragma is in settings.react.

Copy link
Member Author

Choose a reason for hiding this comment

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

form_components and link_components don't seem to be in react but we designed them this way from the beginning.

// <https://github.com/jsx-eslint/eslint-plugin-react#configuration-legacy-eslintrc->
#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema)]
#[cfg_attr(test, derive(PartialEq))]
pub struct ReactPluginSettings {
    #[serde(default = "default_pragma")]
    pub pragma: CompactStr,

    #[serde(default)]
    #[serde(rename = "formComponents")]
    form_components: Vec<CustomComponent>,

    #[serde(default)]
    #[serde(rename = "linkComponents")]
    link_components: Vec<CustomComponent>,
    // TODO: More properties should be added
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

My fault, I missed this. Seems a simple way to fix this issue has merged.

@mysteryven
Copy link
Contributor

Close in favor of #6872

@mysteryven mysteryven closed this Oct 25, 2024
@shulaoda shulaoda deleted the fix/linter-iframe-missing-sandbox branch October 25, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-linter Area - Linter C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

linter: oxc for vscode, document.createElement("iframe") - missing a sandbox attribute

2 participants