Skip to content

Support Angular @let declaration syntax#16474

Merged
sosukesuzuki merged 29 commits intoprettier:mainfrom
sosukesuzuki:feat-let-syntax
Jul 13, 2024
Merged

Support Angular @let declaration syntax#16474
sosukesuzuki merged 29 commits intoprettier:mainfrom
sosukesuzuki:feat-let-syntax

Conversation

@sosukesuzuki
Copy link
Copy Markdown
Contributor

@sosukesuzuki sosukesuzuki commented Jul 13, 2024

Description

Adds support for Angular @let declaration syntax1.

Fixes: #16296

Checklist

  • I’ve added tests to confirm my change works.
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Footnotes

  1. https://blog.angular.dev/introducing-let-in-angular-686f9f383f0f

Comment thread src/language-js/print/assignment.js Outdated
);
}

function printAssignmentWithLayout(
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.

A printAssignmentWithLayout function that allows you to pass rightDoc and layout directly.The logic is the same as the original printAssignment.

Comment thread src/language-html/embed/angular-let-declarations.js Outdated
Comment thread src/language-html/visitor-keys.js Outdated
@sosukesuzuki sosukesuzuki marked this pull request as ready for review July 13, 2024 06:49
@@ -0,0 +1,27 @@
import { group } from "../../document/builders.js";
import { printAssignmentWithLayout } from "../../language-js/print/assignment.js";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't do this before, and I think we can move this to js printer by doing following:

  • create a new __ng_... parser
  • parse right part, then wrap the right node with a manually created VariableDeclaration

Do you think it's reasonable?

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.

IMO, the answer is no. I think the current code is simpler.

First, let me confirm one thing. Is your concern that there will be a dependency from the code for HTML formatting to the code for JavaScript?

If so, would moving printAssignmentWithLayout to src/common solve the your concern? It is just a function that takes leftDoc, operator, rightDoc, and layout and returns a new doc, so it is not specific to JavaScript.

Copy link
Copy Markdown
Member

@fisker fisker Jul 13, 2024

Choose a reason for hiding this comment

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

Is your concern that there will be a dependency from the code for HTML formatting to the code for JavaScript?

Main concern is about plugin, let me check how prettier-plugin-pug works first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Sorry I still don't quite understand the problem, how does my change affect the plugin?

Copy link
Copy Markdown
Member

@fisker fisker Jul 13, 2024

Choose a reason for hiding this comment

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

It doesn't, but I mean if we move all print logic to js printer, it will be easier for plugin to use.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright, I understood the logic wrong, so it's just one line?

group([group(leftDoc), operator, group(indent([line, rightDoc]))]);

Let's put it in HTML printer (maybe with a comment).

The file size grows by import the file https://github.com/prettier/prettier/actions/runs/9918259561/job/27402696804?pr=16474#step:4:298

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.

Sounds good to me

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.

@fisker

This comment was marked as outdated.

Comment thread tests/format/angular/let-declaration/complex.html Outdated
@fisker fisker changed the title Support Angular @let declration syntax Support Angular @let declaration syntax Jul 13, 2024
Comment thread changelog_unreleased/angular/16474.md Outdated
@@ -0,0 +1,19 @@
const snippets = ["@let foo = 'Hello' + ', World'; "];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo in directory name

renovate Bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Aug 21, 2024