Skip to content

Fix broken formatting rules around namespaced JSX attributes #55294

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

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

lyonbot
Copy link
Contributor

@lyonbot lyonbot commented Aug 7, 2023

Fixes #55293

Formatting const a = <div: aa foo : bar="test" foo:baz="5" />;

into const a = <div:aa foo:bar="test" foo:baz="5" />;

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 7, 2023
@lyonbot
Copy link
Contributor Author

lyonbot commented Aug 7, 2023

A simple script to validate the changes:

const ts = require("./built/local/typescript");
const fs = require("fs");

const options = ts.getDefaultCompilerOptions();
options.jsx = ts.JsxEmit.React;

const mockName = "file.tsx";
const mockContent = `
const a = <div: aa foo : bar="test" foo:baz="5" />;
`;
fs.writeFileSync(mockName, mockContent);


/** @type {ts.LanguageServiceHost} */
const servicesHost = {
  getScriptFileNames: () => [mockName],
  getScriptVersion: fileName => "1",
  getScriptSnapshot: fileName => {
    if (!fs.existsSync(fileName)) return undefined;
    return ts.ScriptSnapshot.fromString(fs.readFileSync(fileName).toString());
  },
  getCurrentDirectory: () => process.cwd(),
  getCompilationSettings: () => options,
  getDefaultLibFileName: options => ts.getDefaultLibFilePath(options),
  fileExists: ts.sys.fileExists,
  readFile: ts.sys.readFile,
  readDirectory: ts.sys.readDirectory,
  directoryExists: ts.sys.directoryExists,
  getDirectories: ts.sys.getDirectories,
};

const s = ts.createLanguageService(servicesHost, ts.createDocumentRegistry());

const errors = s.getSyntacticDiagnostics(mockName);
console.log("errors", errors.map(i => `err: ${i.start}-${i.length} [${i.code}] ${i.messageText}`));

const eds = s.getFormattingEditsForDocument(mockName, ts.getDefaultFormatCodeSettings());

let sourceText = mockContent;
let offset = 0;
for (const ed of eds) {
  console.log("ed: ", ed);
  sourceText = sourceText.slice(0, ed.span.start + offset) + ed.newText + sourceText.slice(ed.span.start + ed.span.length + offset);
  offset += ed.newText.length - ed.span.length;
  console.log(" --> ", sourceText);
}

Outputs:

errors []
ed:  { span: { start: 16, length: 1 }, newText: '' }
 -->  
const a = <div:aa foo : bar="test" foo:baz="5" />;

ed:  { span: { start: 23, length: 1 }, newText: '' }
 -->  
const a = <div:aa foo: bar="test" foo:baz="5" />;

ed:  { span: { start: 25, length: 1 }, newText: '' }
 -->  
const a = <div:aa foo:bar="test" foo:baz="5" />;

@jakebailey
Copy link
Member

You'll need to write a test or two; there should be some examples in the fourslash test directory which call formatting.

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 7, 2023
@lyonbot
Copy link
Contributor Author

lyonbot commented Aug 7, 2023

@microsoft-github-policy-service agree

@lyonbot
Copy link
Contributor Author

lyonbot commented Aug 7, 2023

@jakebailey hello, i've added the test :)

@lyonbot lyonbot changed the title Remove space after colon in JSX introduced by formatting Fix broken formatting rules around namespaced JSX attributes Aug 8, 2023
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems fine to be but @andrewbranch may know any gotchas.

@jakebailey jakebailey requested a review from andrewbranch August 9, 2023 19:59
@jakebailey jakebailey merged commit b35fa04 into microsoft:main Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unwanted space after the colon of a namespaced JSX attribute
4 participants