Add prettier config to support format-on-save #2323

stufreen · 2023-02-22T19:36:22Z

PR Summary:

Adds Prettier config file so that contributors will have the same code formatting configuration by default.

Why are these changes introduced?

Fixes https://github.com/Shopify/dawn-private/issues/105

Using an auto-formatter will help keep the code style uniform and speed up development.

What approach did you take?

My process was the following:

Add prettierrc.json file with settings that matched up closely with our existing code
Added workspace VSCode settings to the .vscode directory to help VSCode users get set up quickly
Saved all files in the assets directory to auto-format them

Other considerations

Decision log

#	Decision	Alternatives	Rationale	Downsides
1	Run all css and js files through prettier	Do this on an ad-hoc basis in future PRs	If we don't do this we'll likely see a lot changes in future PRs that are unrelated to the code being changed. Better to have all of the noise in this PR.	This PR is very large and has some changes to almost every file. Any work in progress will need to be rebased which may be non-trivial.

Visual impact on existing themes

There should be no changes.

Generally speaking the changes fall in to a few categories:

Double quote to single quote
Add 0 to decimals, e.g. .13 becomes 0.13
Add commas to final items in arrays
Brackets around arrow function params (e.g. e => { becomes (e) => {)
Indentation changes

Testing steps/scenarios

The changes should all be inconsequential, but smoke testing is recommended. Ensure no new console errors are logged or other issues detected in the inspector.

Demo links

Store
Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

stufreen · 2023-02-22T20:33:07Z

.prettierrc.json

@@ -0,0 +1,8 @@
+{
+  "printWidth": 120,


This seemed to be the least disruptive in our codebase. Prettier docs recommend 80 but I found that it resulted in many changes (e.g. global.js increased by 200 lines)

I'd recommend at least 120. 80 seems like a setting from the 90s (no offense to Prettier team!) I recently started using 140 and can see it being better option (for me obviously)

I don't really mind 120. The liquid prettier plugin defaults to 120 as well, so the consistency there is kind of nice. But we can always make file type exceptions if we liked 80 for css but 120 for js.

stufreen · 2023-02-22T20:36:36Z

@KaichenWang @kmeleta Any thoughts on this so far? This config seems to result in not too many changes, and most of the changes it introduces are from inconsistencies in our code style.

If we add the prettier config file should we go ahead and format every asset? I think so, since otherwise we'll be having PRs for weeks with prettier changes.

stufreen · 2023-02-22T20:41:44Z

.vscode/extensions.json

@@ -1,3 +1,3 @@
 {
-  "recommendations": ["shopify.theme-check-vscode"]
+  "recommendations": ["shopify.theme-check-vscode", "esbenp.prettier-vscode"]


This will prompt users to install the Prettier plugin if they're using VSCode.

stufreen · 2023-02-22T20:42:24Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+  "editor.formatOnSave": true


I'm on the fence about this -- if the user has a different formatter installed it might screw up.

"We" also recommend [liquid]-specific settings which includes Shopify Prettier formatter. I don't think this will be an issue.

I'd like to keep it. If it becomes an issue we can always change it.

Though if we keep this and didn't want to format all our files as part of this PR for some reason, we could consider adding editor.formatOnSaveMode": "modifications" to limit the scope of the changes as we go.

vfonic

I'd urge you to run the formatter on all files within this PR. 🙏

Not sure if my approval is worth anything (I'm not Shopify employee)

vfonic · 2023-02-24T17:35:47Z

.prettierrc.json

@@ -0,0 +1,8 @@
+{
+  "printWidth": 120,


I'd recommend at least 120. 80 seems like a setting from the 90s (no offense to Prettier team!) I recently started using 140 and can see it being better option (for me obviously)

vfonic · 2023-02-24T17:38:17Z

.prettierrc.json

+  "tabWidth": 2,
+  "useTabs": false,
+  "semi": true
+}


trailingComma default is "es5": https://prettier.io/docs/en/options.html#trailing-commas
tabWidth default is 2: https://prettier.io/docs/en/options.html#tab-width
useTabs default is false: https://prettier.io/docs/en/options.html#tabs
semi default is true: https://prettier.io/docs/en/options.html#semicolons

I don't think these defaults recently (or ever) changed so you can omit them, but you can also keep them if you think it shouldn't be implicit.

vfonic · 2023-02-24T17:42:13Z

.vscode/settings.json

@@ -0,0 +1,3 @@
+{
+  "editor.formatOnSave": true


"We" also recommend [liquid]-specific settings which includes Shopify Prettier formatter. I don't think this will be an issue.

vfonic · 2023-02-24T17:45:07Z

assets/global.js

-    this.querySelectorAll('summary').forEach(summary => summary.addEventListener('click', this.onSummaryClick.bind(this)));
-    this.querySelectorAll('button').forEach(button => button.addEventListener('click', this.onCloseButtonClick.bind(this)));
+    this.querySelectorAll('summary').forEach((summary) =>
+      summary.addEventListener('click', this.onSummaryClick.bind(this))
+    );
+    this.querySelectorAll('button').forEach((button) =>
+      button.addEventListener('click', this.onCloseButtonClick.bind(this))
+    );


These, previously 2, now 6 lines, are 124 and 125 long respectively. These both would fit under 140 printWidth. Just sayin' (your teammates already used, without formatter, 120+ printWidth)

I actually don't find the single line version of these lines to be very readable though. Without any formatting enforce otherwise, I'd opt to break those lines right at the fat arrow myself.

vfonic · 2023-02-24T17:46:41Z

assets/global.js

-      if (this.mainDetailsToggle.hasAttribute('open') && !this.mainDetailsToggle.contains(document.activeElement)) this.closeMenuDrawer();
+      if (this.mainDetailsToggle.hasAttribute('open') && !this.mainDetailsToggle.contains(document.activeElement))
+        this.closeMenuDrawer();


This is 139 chars long. Fits within 140 printWidth

140 seemed a bit long when I was experimenting (it created a few mega-long logic statements and CSS rules), but 120 seems to fit in well with existing line lengths.

kmeleta · 2023-02-24T18:15:23Z

I think what you have is looking good. As mentioned above, all or most of those appear to be the defaults so we can remove those as long as that still takes preference over any user-specific settings (which I believe is the case). Not sure if there's any significant motivation to be explicit with them other than easy reference.

If we add the prettier config file should we go ahead and format every asset?

Yeah ideally. We eventually did that with the liquid prettier, because the random formatting in subsequent PRs became distracting. But we batched those changes into several PRs so we could more easily test for regressions, given that it wasn't prod-tested at the time. I wouldn't think we have the same concern prettifying css and js.

KaichenWang

Looks good! Thanks for doing this!

kmeleta

Everything looks to me. Thanks Stu

Run base.css through prettier Run global.js through prettier Add prettier to list of recommended settings and enable format on save Prettify base and global again Remove unnecessary params for prettierrc Run all files through prettier Fix rule re-added by bad rebase Revery mask-blobs (don't prettier it)

* shopify/main: [Video] Add fade in on scroll animation (Shopify#2495) Animate rich text and email signup (Shopify#2408) [Gift card] Add help doc link and change label for translation (Shopify#2471) Add prettier config to support format-on-save (Shopify#2323) Announcements slideshow (Shopify#2394) fix: Update Follow on Shop Option Text (Shopify#2463) Enable "per-PR" async PR mode (Shopify#2488) Animate multicolumn (Shopify#2409) Fix improper image sizes in the collage section (Shopify#2478) Replaced depreciated liquid property/value (Shopify#2480)

* next: [Video] Add fade in on scroll animation (Shopify#2495) Animate rich text and email signup (Shopify#2408) [Gift card] Add help doc link and change label for translation (Shopify#2471) Add prettier config to support format-on-save (Shopify#2323) Announcements slideshow (Shopify#2394) fix: Update Follow on Shop Option Text (Shopify#2463) Enable "per-PR" async PR mode (Shopify#2488) Animate multicolumn (Shopify#2409) Fix improper image sizes in the collage section (Shopify#2478) Replaced depreciated liquid property/value (Shopify#2480) Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next Update from Shopify for theme dawn/next

* Add prettier config file * Prettify JS and CSS files Run base.css through prettier Run global.js through prettier Add prettier to list of recommended settings and enable format on save Prettify base and global again Remove unnecessary params for prettierrc Run all files through prettier Fix rule re-added by bad rebase Revery mask-blobs (don't prettier it)

stufreen commented Feb 22, 2023

View reviewed changes

stufreen requested review from KaichenWang and kmeleta February 22, 2023 20:33

stufreen commented Feb 22, 2023

View reviewed changes

vfonic approved these changes Feb 24, 2023

View reviewed changes

stufreen force-pushed the stu/prettier-config branch from f2407c8 to c9a5f7c Compare March 31, 2023 18:59

stufreen marked this pull request as ready for review March 31, 2023 19:44

vfonic mentioned this pull request Apr 1, 2023

Remove unused image-banner section setting translation #2482

Merged

9 tasks

KaichenWang approved these changes Apr 3, 2023

View reviewed changes

kmeleta approved these changes Apr 4, 2023

View reviewed changes

Stu Freen added 2 commits April 4, 2023 13:30

Add prettier config file

3af8cd4

stufreen force-pushed the stu/prettier-config branch from 72645b8 to 5d95e0e Compare April 4, 2023 17:34

stufreen merged commit 1880145 into main Apr 4, 2023

stufreen deleted the stu/prettier-config branch April 4, 2023 17:35

kmeleta mentioned this pull request Apr 7, 2023

Prettier and vscode settings tweaks #2503

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prettier config to support format-on-save #2323

Add prettier config to support format-on-save #2323

stufreen commented Feb 22, 2023 •

edited

Loading

stufreen Feb 22, 2023

vfonic Feb 24, 2023

kmeleta Feb 24, 2023

stufreen commented Feb 22, 2023

stufreen Feb 22, 2023

stufreen Feb 22, 2023

vfonic Feb 24, 2023 •

edited

Loading

kmeleta Feb 24, 2023

kmeleta Feb 24, 2023

vfonic left a comment

vfonic Feb 24, 2023

vfonic Feb 24, 2023 •

edited

Loading

vfonic Feb 24, 2023 •

edited

Loading

vfonic Feb 24, 2023

kmeleta Feb 24, 2023

vfonic Feb 24, 2023

stufreen Feb 24, 2023 •

edited

Loading

kmeleta commented Feb 24, 2023

KaichenWang left a comment

kmeleta left a comment

Add prettier config to support format-on-save #2323

Add prettier config to support format-on-save #2323

Conversation

stufreen commented Feb 22, 2023 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Other considerations

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stufreen commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

vfonic Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stufreen Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

kmeleta commented Feb 24, 2023

KaichenWang left a comment

Choose a reason for hiding this comment

kmeleta left a comment

Choose a reason for hiding this comment

stufreen commented Feb 22, 2023 •

edited

Loading

vfonic Feb 24, 2023 •

edited

Loading

vfonic Feb 24, 2023 •

edited

Loading

vfonic Feb 24, 2023 •

edited

Loading

stufreen Feb 24, 2023 •

edited

Loading