Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

NullVoxPopuli · 2023-07-04T22:57:54Z

I haven't explained anything well in this PR, so I'm going to split it out:

Upgrade lints in this package: Lint upgrade only #175
Upgrade lint in the blueprint:
- Upgrade prettier and prettier-plugin-ember-template-tag #180 (reveals the bulk of why this (Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138) PR exists
Ensure we don't emit lint errors: (pending)
Split out prettier from eslint: (pending)

previous description

previous blockers

~~Blocked by: #136~~
~~(due to the babel plugin rest spread issue #133 )~~

Blocked by : ember-cli/ember-cli#10334

This is no longer blocked because we can.. just emit files with no problems

Most of this PR is formatting / lint changes as a result of running :fix
Also lockfile,

tl;dr:

update prettier to v3 (otherwise prettier deletes gts files)
update prettier-plugin-ember-template-tag to 1.0.2
update the lint meta-package for compat with prettier v3 / prettier-as-a-separate-command
rename .prettierrc.js to .prettierrc.cjs (required by prettier v3)
format all tests/fixtures to match prettier expectations (because ember addon doesn't run lint:fix for us)
fix formatting in files/*/tsconfig.json
fix formatting in files/*/rollup.config.mjs
- when TS is not present, the changes are no-ops -- it's just simpler for us to maintain this way.

We already have prettier config in the right place: https://github.com/embroider-build/addon-blueprint/blob/main/files/__addonLocation__/.prettierrc.cjs
But eslint-plugin-prettier doesn't work with eslint-plugin-ember until this is merged and released: ember-cli/eslint-plugin-ember#1920

Running prettier via eslint is way slower than running prettier separately, so I'd like to propose running it as a separate command for the addon.
(which we do via switching from eslint-plugin-prettier to eslint-config-prettier, which only disables the formatting rules in eslint that prettier is concerned with)

OF NOTE: To have things working with gjs/gts, I had to upgrade to prettier 3, the prettier-plugin-ember-template-tag plugin (post 1.0) is not compat with prettier < 3.

Also, I formatted all the fixtures so that prettier -c would pass during our lint checks in the tests.

I've been testing with

EMBER_CLI_PNPM=true npx ember-cli@beta addon test-addon -b ../OpenSource/emberjs/addon-blueprint/ --pnpm

history

When reviewing: #136 (comment)
I found out we don't run prettier anywhere in the default blueprint 😅

When running prettier in the addon,

we have a working prettier situation when using --addon-only
we have a working prettier situation when using the default / monorepo setup
if someone wants to run prettier from the root of their monorepo, nested configs do get picked up by prettier but my editor (neovim) doesn't respect nested configs -- vscode does tho -- so I need to figure out what's going on with my neovim setup

I think this approach to configuring prettier for the addon is good because:

we defer to the app blueprint for everything about the app, we (as a composite blueprint), don't know how it will want to lint itself, so deferring to each workspace via pnpm --filter '*' lint feels like a good thing.

If anyone has ideas about better options, I'm open to that, too
(in part, the approach in this PR is "what I'm used to", so I could be biased)

The first commit here is an error, which shows that adding lint:prettier and lint:prettier:fix are now running, showing that the addon did not pass prettier on a fresh addon/monorepo setup

The second commit sets up an ignore file so we don't run prettier on the compiled output / dist / etc

files/__addonLocation__/package.json

files/.prettierignore

…e the formatting rules eslint has

…ttier so that we can reduce lint:fix differences in the generated output

…ier at the same time

NullVoxPopuli · 2023-08-14T13:47:07Z

files/__addonLocation__/rollup.config.mjs

@@ -15,7 +15,11 @@ export default {
  plugins: [
    // These are the modules that users should be able to import from your
    // addon. Anything not listed here may get optimized away.
-    addon.publicEntrypoints(['components/**/*.js', 'index.js'<% if (typescript) {%>, 'template-registry.js'<% } %>]),
+    addon.publicEntrypoints([


important change -- no more typescript branches in rollup. there is no downside to having this stuff here

Not sure I agree with this change...

First, seems unrelated to the PR's primary purpose?

Then, I would find it confusing to see a file referenced that does not exist, and that does not even have any use (in a JS addon). Would be different if it were reference implicitly through some glob like *.js, but here we have an explicit reference to this specific file.

I'd rather have a bit of complexity on our side, than cause confusion to the user...

it's related in that we emit a prettier error in TS projects, but I think I can just wrap the whole line rather than have the typescript condition inline

NullVoxPopuli · 2023-08-14T14:21:41Z

.npmignore

@@ -0,0 +1,2 @@
+/.eslintrc.cjs


this is needed because eslint-plugin-n warns us when we require from a dependency in a file that we ship to npm and that dependency is not declared in "dependencies".

NullVoxPopuli · 2023-08-14T14:23:01Z

.prettierignore

@@ -1 +1,8 @@
-tests/fixtures/
+# Blueprint files
+# We want the blueprint to run lint:fix on these


The beginning of this is here: ember-cli/ember-cli#10334

For now, and what this PR mostly does is, we emit only already correct files. This is brittle over time (as is proven by the reason lint:fix got added to the app blueprint), but it'll get us unblocked in the short term.

NullVoxPopuli · 2023-08-14T14:23:19Z

.prettierrc.cjs

@@ -0,0 +1,19 @@
+'use strict';


file renamed from 'js' -- should not be "new"

NullVoxPopuli · 2023-08-14T14:25:44Z

index.js

@@ -38,7 +38,7 @@ module.exports = {
    if (!options.addonOnly) {
      if (fs.existsSync(path.join('..', 'package.json'))) {
        options.ui.writeInfoLine(
-          "Existing monorepo detected! The blueprint will only create the addon and test-app folders, and omit any other files in the repo's root folder."
+          "Existing monorepo detected! The blueprint will only create the addon and test-app folders, and omit any other files in the repo's root folder.",


in prettier 3, these trailing commas are default

NullVoxPopuli · 2023-08-14T14:26:37Z

tests/helpers/utils.ts

 import fs from 'node:fs/promises';
 import os from 'node:os';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';

+import { execa, type Options } from 'execa';


the lint meta-package groups all the node imports together, separate from external deps

What exactly are you referring to as the "lint meta-package"?

It'll be easier to see here (I just extracted this, in response to your comments): #175 (comment)

simonihmig

Although you have a lot of text in your description (maybe due to this? 🙃), I am not quite getting what the primary intent of this PR is?

Is it "just" regular maintenance (upgrading prettier)? Or is upgrading the means to fix a real problem? Like what label are we going to apply to this PR (for the lerna-changelog), bug or enhancement? 😏

To have things working with gjs/gts, I had to upgrade to prettier 3, the prettier-plugin-ember-template-tag plugin (post 1.0) is not compat with prettier < 3.

This seems to refer to a problem we have. So do we have a problem? We already used prettier-plugin-ember-template-tag v1.0.0, which did work with prettier v2, didn't it? (e.g. like in https://github.com/CrowdStrike/ember-headless-form)

Running prettier via eslint is way slower than running prettier separately, so I'd like to propose running it as a separate command for the addon.

This is something I am a bit concerned, or need clarification...

First, on the performance issue, is this when letting prettier format things (which happens very frequently, but usually using prettier directly in your IDE rather then through eslint), or when "style-linting"? In the latter case, is the performance penalty really relevant (given you don't do this very frequently)? Any numbers?

Then, is splitting prettier from eslint a stop-gap solution which we will revert later (when ember-cli/eslint-plugin-ember#1920 is merged), or is this the way to go anyway? If the latter, is this aligned to what is happening in other blueprints (v1 addon and app)? Should we align? (I think we should)

simonihmig · 2023-08-16T07:26:07Z

files/__addonLocation__/rollup.config.mjs

@@ -15,7 +15,11 @@ export default {
  plugins: [
    // These are the modules that users should be able to import from your
    // addon. Anything not listed here may get optimized away.
-    addon.publicEntrypoints(['components/**/*.js', 'index.js'<% if (typescript) {%>, 'template-registry.js'<% } %>]),
+    addon.publicEntrypoints([


Not sure I agree with this change...

First, seems unrelated to the PR's primary purpose?

Then, I would find it confusing to see a file referenced that does not exist, and that does not even have any use (in a JS addon). Would be different if it were reference implicitly through some glob like *.js, but here we have an explicit reference to this specific file.

I'd rather have a bit of complexity on our side, than cause confusion to the user...

simonihmig · 2023-08-16T07:27:30Z

files/__addonLocation__/rollup.config.mjs

@@ -34,7 +38,7 @@ export default {
    // By default, this will load the actual babel config from the file
    // babel.config.json.
    babel({
-      extensions: ['.js', '.gjs'<% if (typescript) { %>, '.ts', '.gts'<% } %>],
+      extensions: ['.js', '.gjs', '.ts', '.gts'],


similar to the above, but to a lesser degree. Still: what do we gain here?

simonihmig · 2023-08-16T07:30:03Z

tests/fixtures/typescript/test-app/tests/rendering/template-only-test.ts

@@ -3,12 +3,12 @@ import { setupRenderingTest } from 'ember-qunit';
 import { render } from '@ember/test-helpers';
 import { hbs } from 'ember-cli-htmlbars';

-module('Rendering | template-only', function(hooks) {
+module('Rendering | template-only', function (hooks) {


Uh, having used JS for what? 25 years?, I will need to get used to this style change... 😅

simonihmig · 2023-08-16T07:30:58Z

tests/helpers/utils.ts

 import fs from 'node:fs/promises';
 import os from 'node:os';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';

+import { execa, type Options } from 'execa';


What exactly are you referring to as the "lint meta-package"?

NullVoxPopuli · 2023-08-16T11:46:04Z

I updated this PR's description with how I'm going to attempt to break things up in this PR: #138 (comment)

NullVoxPopuli · 2023-08-17T12:01:08Z

evidence here: https://github.com/embroider-build/addon-blueprint/actions/runs/5890531482/job/15975827606?pr=180#step:5:1475
From: #180

[fix:js] ESLint: 8.47.0
[fix:js] 
[fix:js] TypeError: prettier.resolveConfig.sync is not a function
[fix:js] Occurred while linting /tmp/v2-addon-blueprint--iOTzcu/my-addon/test-app/.eslintrc.js:1
[fix:js] Rule: \"prettier/prettier\"
[fix:js]     at Program (/tmp/v2-addon-blueprint--iOTzcu/my-addon/node_modules/eslint-plugin-prettier/eslint-plugin-prettier.js:138:40)
[fix:js]     at ruleErrorHandler (/tmp/v2-addon-blueprint--iOTzcu/my-addon/node_modules/eslint/lib/linter/linter.js:1050:28)
[fix:js]     at /tmp/v2-addon-blueprint--iOTzcu/my-addon/node_modules/eslint/lib/linter/safe-emitter.js:45:58

will try upgrading more prettier-related dependencies to see if that helps

NullVoxPopuli · 2023-08-17T12:04:23Z

First, on the performance issue, is this when letting prettier format things (which happens very frequently, but usually using prettier directly in your IDE rather then through eslint), or when "style-linting"? In the latter case, is the performance penalty really relevant (given you don't do this very frequently)? Any numbers?

It's only when ran through eslint -- eslint is significantly slower when it also runs prettier. A lot of people have their editors set up to format on save, or lint:fix/check their enitre monorepos, which can take more than a minute, and multiple minutes when prettier is integrated in to eslint.

NullVoxPopuli · 2023-08-17T12:10:23Z

Looks like upgrading dependencies fixed that error for yarn and pnpm, but npm -- #180 (comment)

patricklx · 2023-08-17T12:21:19Z

I like having prettier with eslint, as it reports the actual issues instead of just giving me the did you forget to run prettier?...
and I also sometimes do not want to checkout a project to make small changed. I like to edit code in the github editor :) and there I need to actual issues reported

NullVoxPopuli · 2023-08-17T13:12:13Z

as it reports the actual issues instead of just giving me the did you forget to run prettier?

why do you need to know the issues?

I like to edit code in the github editor :) and there I need to actual issues reported

github editor integrates with linters?

in any case, if you know how to fix the problem with the npm tests in #180 that would be most helpful, because then we don't need to (or can delay) explore(ing) splitting out the commands

NullVoxPopuli · 2023-08-17T13:35:37Z

I just witnessed another project using prettier 2 get their gts files deleted 😬

simonihmig · 2023-08-18T23:36:11Z

I just witnessed another project using prettier 2 get their gts files deleted

Do we have an explanation why this actually happens? Like I could understand that something makes prettier cause unwanted content changes, but delete the whole file?

NullVoxPopuli · 2023-08-19T03:19:36Z

@patricklx could probably explain it better (but it's the trifecta of eslint-plugin-prettier, prettier 2, and prettier-plugin-ember-template-tag > 1.0.0)

The fix for prettier 2 (downgrading prettier-plugin-ember-template-tag to < v1) has been merged and released, and I have a separate set of PRs that upgrades to prettier 3 and removes eslint-plugin-prettier.

I think I want to discuss setting up turbo (no cache) tho... because the main downside to splitting up prettier from eslint is that eslint could run after prettier, and mess up prettier's formatting, so prettier needs to run after eslint, and that's best orchestrated via turbo.

Gonna close this PR for now

NullVoxPopuli mentioned this pull request Jul 4, 2023

Move off rollup-plugin-ts #136

Merged

NullVoxPopuli force-pushed the prettier-in-addon branch from 57cc297 to abcc834 Compare August 8, 2023 15:43

mansona reviewed Aug 8, 2023

View reviewed changes

files/__addonLocation__/package.json Show resolved Hide resolved

NullVoxPopuli changed the title ~~Try prettier in the addon~~ Run prettier separately, in the addon, without eslint-plugin-prettier Aug 8, 2023

NullVoxPopuli force-pushed the prettier-in-addon branch from 6587910 to 31de726 Compare August 9, 2023 16:44

NullVoxPopuli commented Aug 10, 2023

View reviewed changes

files/.prettierignore Outdated Show resolved Hide resolved

NullVoxPopuli force-pushed the prettier-in-addon branch 2 times, most recently from 862b43b to 9ae5849 Compare August 10, 2023 11:49

Swap eslint-plugin-prettier for eslint-config-prettier to only disabl…

4662a16

…e the formatting rules eslint has

NullVoxPopuli force-pushed the prettier-in-addon branch from 9ae5849 to 4662a16 Compare August 14, 2023 12:55

NullVoxPopuli added 7 commits August 14, 2023 08:57

Fix prettier dep versions

241d346

Upgrade to prettier 3, because our prettier must match the output pre…

b963ed1

…ttier so that we can reduce lint:fix differences in the generated output

Add concurrently and scripts so that we can run both eslint and prett…

730accb

…ier at the same time

Fix all the formatting mistakes we've accumulated

acfa9b2

Run lint:fix on our own files

68d1444

Fix the tsconfig and rollup config

4a6c16e

Upgrade the lint meta package for prettier v3 compat

86b3b3b

NullVoxPopuli commented Aug 14, 2023

View reviewed changes

NullVoxPopuli marked this pull request as ready for review August 14, 2023 13:52

NullVoxPopuli commented Aug 14, 2023

View reviewed changes

.prettierrc.cjs

@@ -0,0 +1,19 @@

'use strict';

Copy link

Collaborator Author

NullVoxPopuli Aug 14, 2023

Choose a reason for hiding this comment

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

file renamed from 'js' -- should not be "new"

Prettier doesn't try to lint .ember-try extensions

f3c2bc7

NullVoxPopuli commented Aug 14, 2023

View reviewed changes

NullVoxPopuli requested review from achambers, simonihmig and void-mAlex August 14, 2023 14:34

NullVoxPopuli changed the title ~~Run prettier separately, in the addon, without eslint-plugin-prettier~~ Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier Aug 14, 2023

simonihmig reviewed Aug 16, 2023

View reviewed changes

NullVoxPopuli mentioned this pull request Aug 16, 2023

Lint upgrade only #175

Merged

NullVoxPopuli mentioned this pull request Aug 17, 2023

Upgrade prettier and prettier-plugin-ember-template-tag #180

Merged

NullVoxPopuli mentioned this pull request Aug 17, 2023

Run prettier separately #183

Draft

NullVoxPopuli closed this Aug 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

NullVoxPopuli commented Jul 4, 2023 •

edited

NullVoxPopuli Aug 14, 2023

simonihmig Aug 16, 2023

NullVoxPopuli Aug 16, 2023

NullVoxPopuli Aug 14, 2023

NullVoxPopuli Aug 14, 2023

NullVoxPopuli Aug 14, 2023

NullVoxPopuli Aug 14, 2023

NullVoxPopuli Aug 14, 2023

simonihmig Aug 16, 2023

NullVoxPopuli Aug 16, 2023

simonihmig left a comment

simonihmig Aug 16, 2023

simonihmig Aug 16, 2023

simonihmig Aug 16, 2023

simonihmig Aug 16, 2023

NullVoxPopuli commented Aug 16, 2023

NullVoxPopuli commented Aug 17, 2023

NullVoxPopuli commented Aug 17, 2023

NullVoxPopuli commented Aug 17, 2023

patricklx commented Aug 17, 2023 •

edited

NullVoxPopuli commented Aug 17, 2023 •

edited

NullVoxPopuli commented Aug 17, 2023 •

edited

simonihmig commented Aug 18, 2023

NullVoxPopuli commented Aug 19, 2023 •

edited

Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

Upgrade to prettier 3, run prettier separately, remove eslint-plugin-prettier #138

Conversation

NullVoxPopuli commented Jul 4, 2023 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig left a comment

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

Choose a reason for hiding this comment

NullVoxPopuli commented Aug 16, 2023

NullVoxPopuli commented Aug 17, 2023

NullVoxPopuli commented Aug 17, 2023

NullVoxPopuli commented Aug 17, 2023

patricklx commented Aug 17, 2023 • edited

NullVoxPopuli commented Aug 17, 2023 • edited

NullVoxPopuli commented Aug 17, 2023 • edited

simonihmig commented Aug 18, 2023

NullVoxPopuli commented Aug 19, 2023 • edited

NullVoxPopuli commented Jul 4, 2023 •

edited

patricklx commented Aug 17, 2023 •

edited

NullVoxPopuli commented Aug 17, 2023 •

edited

NullVoxPopuli commented Aug 17, 2023 •

edited

NullVoxPopuli commented Aug 19, 2023 •

edited