Skip to content

feat: integrate Prettier 3.8.1 for WASM editor and scripts#1563

Open
Divyansh200102 wants to merge 5 commits into
elalish:masterfrom
Divyansh200102:feat/prettier-format
Open

feat: integrate Prettier 3.8.1 for WASM editor and scripts#1563
Divyansh200102 wants to merge 5 commits into
elalish:masterfrom
Divyansh200102:feat/prettier-format

Conversation

@Divyansh200102
Copy link
Copy Markdown
Contributor

Pull Request Description

  • bindings/wasm/examples/editor/package.json: Added prettier ^3.8.1 to enable standalone browser and local formatting.
  • bindings/wasm/examples/editor/editor.js: Implemented lazy-loaded Prettier 3.0+ formatting with undo-stack preservation and cursor/scroll state restoration.
  • scripts/format.sh: Migrated WebAssembly file formatting (.js, .ts, .html) from clang-format to npx prettier.

Screenshot
image

Note: The diff is large because Prettier was run across all relevant files, and they were reformatted according to the new Prettier configuration.

@Divyansh200102 Divyansh200102 marked this pull request as draft March 3, 2026 07:22
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.44%. Comparing base (ffe6c02) to head (2054ec5).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/collider.h 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1563   +/-   ##
=======================================
  Coverage   92.44%   92.44%           
=======================================
  Files          35       35           
  Lines        6098     6098           
=======================================
  Hits         5637     5637           
  Misses        461      461           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pca006132
Copy link
Copy Markdown
Collaborator

Is it possible to split the commit? One commit before the formatting, one commit after that. Otherwise it is hard to read the diff.

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

Divyansh200102 commented Mar 3, 2026

@pca006132 I’ve integrated Prettier 3.8.1 into the editor and updated scripts/format.sh and check_format.yml to use Prettier for JS/TS/HTML instead of clang-format. Because of this the CI is now failing due to these formatting changes as it uses clang-format for checking the format.

Would you prefer proceeding with this full migration to Prettier for consistency between the editor and CI, or should I limit the changes to just the editor button and keep the existing clang-format setup?

If we proceed with this then I'll modfy .github/workflows/check_format.yml with the relevant changes as well.

@Divyansh200102 Divyansh200102 marked this pull request as ready for review March 3, 2026 07:34
@pca006132
Copy link
Copy Markdown
Collaborator

I am fine either way, @elalish and @tonious may have some opinion about which one to use.

I think you should have one commit before formatting, to make the diff readable. Also, it seems that you forgot to add the updated package lock to the commit, and the wasm CI build is failing due to that.

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

Divyansh200102 commented Mar 3, 2026

@pca006132 I have made the commits please review them when you get the time.

Comment thread bindings/wasm/examples/editor/editor.js Outdated
Comment thread bindings/wasm/examples/editor/editor.js Outdated
const tabSize = model.getOptions?.().tabSize ?? 2;

let parser = 'babel';
if (language.includes('typescript')) parser = 'typescript';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we have cases in which the editor is not writing typescript/javascript?

Comment thread bindings/wasm/examples/editor/editor.js Outdated
}

editor.pushUndoStop();
if (viewState) editor.restoreViewState(viewState);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Curious what does this do. I think it is related to undo, but why do we need to do pushUndoStop after applying the edit and restoreViewState? What happens if we remove the second run?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I called pushUndoStop() because according to Monaco docs

"Execute edits on the editor. The edits will land on the undo-redo stack, but no 'undo stop' will be pushed."

Since no undo boundary is created automatically, we must explicitly call pushUndoStop() to create one after formatting.
If we remove the second call, subsequent user edits would continue in the same undo group as the formatting edit instead of being separated.

Copy link
Copy Markdown
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

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

What's the line length limit now? That was a big one we were missing before with Monaco's auto-formatter.

I'm fine with updating the format of all this as long as it's pretty easy to set up VSCode to use prettier to auto-format these files on save. I assume that's no big deal?

Also probably a good idea to add the format commit to git-blame-ignore-revs. @tonious do you have any major branches outstanding? I don't want to create a bunch of conflicts for you.

Comment thread bindings/wasm/examples/editor/editor.js Outdated
} catch (err) {
console.error('Prettier formatting failed:', err);
consoleElement.textContent += `Prettier formatting failed: ${err.message}\r\n`;
consoleElement.scrollTop = consoleElement.scrollHeight;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I believe you can remove these lines if you use console.log instead of console.error.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If it's too much detail to print in the UI, it's reasonable to console.log() a short version, and then console.debug() or console.error() an in depth version. It'll be accessible via the browser console.

Copy link
Copy Markdown
Contributor Author

@Divyansh200102 Divyansh200102 Mar 6, 2026

Choose a reason for hiding this comment

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

I'll keep the console.error(..., err) and console.log(), so the clean string goes to the user's UI, while the full stack trace is still safely dumped into the browser DevTools console for debugging.

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

Divyansh200102 commented Mar 3, 2026

What's the line length limit now? That was a big one we were missing before with Monaco's auto-formatter.

I'm fine with updating the format of all this as long as it's pretty easy to set up VSCode to use prettier to auto-format these files on save. I assume that's no big deal?

Also probably a good idea to add the format commit to git-blame-ignore-revs. @tonious do you have any major branches outstanding? I don't want to create a bunch of conflicts for you.

@elalish The line length limit should be 80 characters, which is Prettier’s default printWidth. We can adjust it if you prefer a different limit. As for using prettier in vs code easily we can add a .prettierrc to ensure consistent settings across different systems. I'll also add the formatting commit to git-blame-ignore-revs as suggested.

@tonious
Copy link
Copy Markdown
Collaborator

tonious commented Mar 3, 2026

Do you have any major branches outstanding? I don't want to create a bunch of conflicts for you.

I've got a branch that's fairly in-depth with show/only. Believe it or not, I usually try to pare down my PRs before I post them -- I'll adjust to the new format at that point.

Comment thread bindings/wasm/examples/editor/editor.js Outdated
Comment on lines +57 to +72
let parser = "babel";
if (language.includes("typescript")) parser = "typescript";

const viewState = editor.saveViewState();
editor.pushUndoStop();

try {
const [{ format }, estree, babel, typescript] = await Promise.all([
import("prettier/standalone"),
import("prettier/plugins/estree"),
import("prettier/plugins/babel"),
import("prettier/plugins/typescript"),
]);

const plugins =
parser === "typescript" ? [typescript, estree] : [babel, estree];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The lazy-loading approach here is interesting. When built, does it end up in a separate bundle?

It looks like there's a bunch of initialization code here that will be rerun every time the user formats their code, including the dynamic imports. Is it reasonable to split the initialization and store format, parser and plugins somewhere?

E.g.: At the top level, something like this:

const prettierWrapper = () => {
    let prettierParts = null;
    const lazyLoad = async () => {
      const [{ format }, estree, babel, typescript] = await Promise.all([
        import("prettier/standalone"),
        import("prettier/plugins/estree"),
        import("prettier/plugins/babel"),
        import("prettier/plugins/typescript"),
      ]);
      return { format, estree, babel, typescript };
    };

    const format = async (originalCode, parser="typescript", options = {}) => {
      if (!prettierParts) {
        prettierParts = await lazyLoad();
      }
      const {format, estree, babel, typescript} = prettierParts;
  
      return await format(originalCode, {
        parser,
        plugins: [parser === "typescript" ? typescript : babel, estree],
        ...options
      });
    };
    return format;
};

const format = prettierWrapper();

Then, in this onclick handler, something like:

  const formatted = await format(
    originalCode,
    language.includes("typescript") ? "typescript" : "babel",
    {
      tabWidth: tabSize,
      singleQuote: true,
      trailingComma: "all",
    }
);

Er, treat this as pseudocode -- it's completely untested 😂

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

Divyansh200102 commented Mar 7, 2026

Can you please review the pr again and see if the changes look good?

image

Also I formated every file in binding/wasm* so I am not sure if that's something I should do or just the format the editor.js file? Please let me if should revert this change or not?

@elalish
Copy link
Copy Markdown
Owner

elalish commented Mar 8, 2026

Also I formated every file in binding/wasm* so I am not sure if that's something I should do or just the format the editor.js file? Please let me if should revert this change or not?

I think that's good - the examples are the main thing that need to be formatted, since we don't want them to change in ManifoldCAD.org when someone hits the format button. I agree that having all of our JS/TS files formatted consistently is also good, so I agree with formatting it all. It also looks like prettier is catching more things than clang-format was.

I'll let @tonious give the final approval here, since he's working on these files the most. I assume it's expected that you'll need to update a few line numbers of stack trace tests due to the formatting changes?

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

Divyansh200102 commented Mar 8, 2026

I assume it's expected that you'll need to update a few line numbers of stack trace tests due to the formatting changes?

yes I just saw the CI jobs and there seems to be some conflicts as well. will push the fix soon.

Copy link
Copy Markdown
Collaborator

@tonious tonious left a comment

Choose a reason for hiding this comment

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

I'm good with this, providing that checks pass, and that this branch is updated to be conflict free.

@elalish elalish self-assigned this Apr 5, 2026
@elalish
Copy link
Copy Markdown
Owner

elalish commented Apr 5, 2026

@Divyansh200102 shall I go ahead and take over this PR? You've done a nice job and I'd like to see it merged.

@Divyansh200102
Copy link
Copy Markdown
Contributor Author

@Divyansh200102 shall I go ahead and take over this PR? You've done a nice job and I'd like to see it merged.

Yes @elalish please go ahead and thank you for stepping in and helping it get merged. Appreciate it.

@elalish
Copy link
Copy Markdown
Owner

elalish commented May 3, 2026

@israrkhan921 This might be up your alley - would you be interested in taking over this PR and pushing it across the finish line?

@israrkhan921
Copy link
Copy Markdown
Contributor

Yes, I can take it, I will check the PR and update you soon, and then start work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants