Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

🐛 Double quotes are not being enforced for JSX string property values #4054

Closed
1 task done
lbartroli opened this issue Dec 14, 2022 · 8 comments · Fixed by #4334
Closed
1 task done

🐛 Double quotes are not being enforced for JSX string property values #4054

lbartroli opened this issue Dec 14, 2022 · 8 comments · Fixed by #4334
Assignees
Labels
A-Formatter Area: formatter L-JSX Language: JSX S-Bug: confirmed Status: report has been confirmed as a valid bug

Comments

@lbartroli
Copy link

Environment information

CLI:
  Version:              11.0.0
  Color support:        true

Platform:
  CPU Architecture:     x86_64
  OS:                   macos

Environment:
  ROME_LOG_DIR:         unset
  NO_COLOR:             unset
  TERM:                 "xterm-256color"

Rome Configuration:
  Status:               loaded
  Formatter disabled:   false
  Linter disabled:      false

Workspace:
  Open Documents:       0

Discovering running Rome servers...

Incompatible Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Rome.

Server:
  Version:              <=10.0.0

Running Rome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ The client isn't connected to any server but rage discovered this running Rome server.

Server:
  Version:              11.0.0
  Name:                 rome_lsp
  CPU Architecture:     x86_64
  OS:                   macos

Workspace:
  Open Documents:       0

Rome Server Log:

⚠ Please review the content of the log file before sharing it publicly as it may contain sensitive information:
  * Path names that may reveal your name, a project name, or the name of your employer.
  * Source code

┐rome_cli::commands::daemon::Running Server{pid=90064}
├─50ms ERROR tower_lsp::transport failed to encode message: failed to encode response: Socket is not connected (os error 57)
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─1ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─0ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::rome/rage{params=RageParams}
├─┘
├─12282ms ERROR tower_lsp::transport failed to encode message: failed to encode response: Socket is not connected (os error 57)
├─┐rome_lsp::server::initialize{params=InitializeParams { process_id: None, root_path: None, root_uri: None, initialization_options: None, capabilities: ClientCapabilities { workspace: None, text_document: None, window: None, general: None, experimental: None }, trace: None, workspace_folders: None, client_info: Some(ClientInfo { name: "rome_service", version: Some("11.0.0") }), locale: None }}
│ ├─0ms INFO rome_lsp::server Starting Rome Language Server...
├─┘
├─┐rome_lsp::server::rome/rage{params=RageParams}
├─┘
├─44659ms ERROR tower_lsp::transport failed to encode message: failed to encode response: Socket is not connected (os error 57)

What happened?

Double quotes on JSX string property values are not being enforced.

image

My rome.json config:

{
    "linter": {
        "enabled": true,
        "rules": {
            "correctness": {
                "noRenderReturnValue": "error",
                "noUnusedVariables": "warn",
                "noUndeclaredVariables": "off"
            },
            "complexity": {
                "useSimplifiedLogicExpression": "warn",
                "noExtraBooleanCast": "warn",
                "useOptionalChain": "warn",
                "noUselessFragments": "warn"
            },
            "style": {
                "noUnusedTemplateLiteral": "warn",
                "noImplicitBoolean": "off",
                "useBlockStatements": "off",
                "useShorthandArrayType": "warn",
                "noNegationElse": "off",
                "useSelfClosingElements": "warn",
                "useTemplate": "warn",
                "useSingleVarDeclarator": "off",
                "noShoutyConstants": "off",
                "useSingleCaseStatement": "off",
                "useFragmentSyntax": "warn"
            },
            "a11y": {
                "useKeyWithClickEvents": "off",
                "useAltText": "off",
                "useValidAnchor": "off"
            },
            "security": {
                "noDangerouslySetInnerHtml": "off"
            },
            "nursery": {
                "useExhaustiveDependencies": "warn",
                "useDefaultParameterLast": "warn",
                "noHeaderScope": "off",
                "noInvalidConstructorSuper": "off",
                "noBannedTypes": "warn",
                "useConst": "warn"
            },
            "suspicious": {
                "noDoubleEquals": "warn",
                "noShadowRestrictedNames": "warn",
                "noAsyncPromiseExecutor": "warn",
                "useValidTypeof": "error",
                "noExplicitAny": "off",
                "noArrayIndexKey": "warn"
            },
            "performance": {
                "noDelete": "warn"
            }
        },
        "ignore": [
            "node_modules/**",
            "coverage",
            "dist-en",
            "./src/graphql/index.ts",
            "./src/graphql/runbooks/generated.ts",
            "./src/graphql/slo/index.ts",
            "./src/graphql/slo-timeseries/index.ts",
            "./src/utils/monaco/workers/**"
        ]
    },
    "formatter": {
        "enabled": true,
        "ignore": [
            "node_modules/**",
            "coverage",
            "dist-en",
            "./src/graphql/index.ts",
            "./src/graphql/runbooks/generated.ts",
            "./src/graphql/slo/index.ts",
            "./src/graphql/slo-timeseries/index.ts",
            "./src/utils/monaco/workers/**"
        ],
        "lineWidth": 120,
        "indentStyle": "space",
        "indentSize": 4
    },
    "javascript": {
        "formatter": {
            "quoteStyle": "single"
        }
    }
}

Expected result

Formatter should enforce double quotes for JSX string property values:

image

Code of Conduct

  • I agree to follow Rome's Code of Conduct
@lbartroli lbartroli added the S-To triage Status: user report of a possible bug that needs to be triaged label Dec 14, 2022
@MichaReiser MichaReiser added A-Formatter Area: formatter S-To triage Status: user report of a possible bug that needs to be triaged L-JSX Language: JSX S-Bug: confirmed Status: report has been confirmed as a valid bug and removed S-To triage Status: user report of a possible bug that needs to be triaged labels Dec 15, 2022
@nissy-dev
Copy link
Contributor

I'll take this issue.

@nissy-dev nissy-dev self-assigned this Mar 12, 2023
@nissy-dev nissy-dev added S-Needs info Status: the issue needs more info in order to be triaged and removed S-Bug: confirmed Status: report has been confirmed as a valid bug A-Formatter Area: formatter S-To triage Status: user report of a possible bug that needs to be triaged L-JSX Language: JSX labels Mar 19, 2023
@nissy-dev nissy-dev added A-Formatter Area: formatter L-JSX Language: JSX and removed S-Needs info Status: the issue needs more info in order to be triaged labels Mar 20, 2023
@nissy-dev
Copy link
Contributor

@denbezrukov Oh, sorry for misunderstanding and thank you for pointing out my mistake 🙇

I guess that the problem that the prettier replace a single quote with a double quote.

I agreed your comment and tried to look into the reason.
It seems the reason that prettier enforces double quotes for jsx attributes is just the past convention based on HTML.

ref: prettier/prettier#73 (comment)

If we focus on prettier compatibilities, we should fix this issue. But If not, we don't need to fix.

@nissy-dev nissy-dev added the S-Bug: confirmed Status: report has been confirmed as a valid bug label Mar 20, 2023
@denbezrukov
Copy link
Contributor

@nissy-dev no worries!

it seems the prettier has an option for jsx quotes.
https://prettier.io/docs/en/options.html#jsx-quotes

#2465 (comment) Rome discussion

@nissy-dev
Copy link
Contributor

@denbezrukov Thank you for sharing Rome discussion!

I understood Rome does not attempt to support this option and format as close as possible to prettier's default formatting. ref: #2465 (reply in thread) Therefore, we should fix this issue?

@denbezrukov
Copy link
Contributor

It's interesting.

So we can fix it:

  • use the quote option for jsx
  • add a new option for jsx quotes

I'm leaning towards the first option:
#2465 (reply in thread)

What do you think?

@nissy-dev
Copy link
Contributor

Sorry for a late reply 🙇

use the quote option for jsx

You mean that formatter follow javascript.formatter.quoteStyle option when treating JSX string property values ? If so, I also agree with the first option 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter L-JSX Language: JSX S-Bug: confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants