Skip to content
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

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

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Aug 17, 2023

gets the latest patches.

As-is, I've seen gts files just.... have their contents be deleted without this change.

Resolves: #184

However, this issue is still open: ember-tooling/prettier-plugin-ember-template-tag#113
But that's independent of this repo.

People are working on it though: prettier/prettier#15079

"a" workaround: ember-tooling/prettier-plugin-ember-template-tag#113 (comment)

resolved problems

BLOCKED: ember-cli's app blueprint needs to use prettier 3 before this PR will succeed

No longer blocked: https://github.com/ember-cli/ember-cli/blob/master/blueprints/app/files/package.json#L102

After upgrading prettier, prettier-plugin-ember-template-tag, eslint-plugin-prettier, and eslint-config-prettier, this problem remains:
#180 (comment)

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Aug 17, 2023

as of 07f6666, for the npm scenarios

 [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--D0JXbz/my-addon/test-app/.eslintrc.js:1
[fix:js] Rule: \"prettier/prettier\"
[fix:js]     at Program (/tmp/v2-addon-blueprint--D0JXbz/my-addon/node_modules/eslint-plugin-prettier/eslint-plugin-prettier.js:138:40)

It seems that this is inescapable without splitting eslint and prettier into separate commands (because then the plugin isn't needed).
This work was already done here: #138
So resolution will happen either in this (#180) or over there in #138 -- depending on whichever is easier to rebase.

Additionally,

  • eslint runs way slower when prettier is integrated
  • prettier recommends that people run it separately
  • format on save works out of the box with prettier in most editors

files/package.json Outdated Show resolved Hide resolved
@patricklx
Copy link

patricklx commented Aug 17, 2023

hi, why is lint only run for npm? anyway, it looks like eslint-plugin-prettier is not directly installed, but indirectly by ember-cli. but its 4.x
you need to install eslint-plugin-prettier@^5 in files/package.json

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Aug 17, 2023

it looks like eslint-plugin-prettier is not directly installed

v5 is present in the addon's package.json

https://github.com/embroider-build/addon-blueprint/pull/180/files#diff-931f0024f6b4877d9d8516d7f8f63f4f854d73614de449600db128823902a82dL78

@NullVoxPopuli
Copy link
Collaborator Author

why would it need to? that's the project root, which doesn't run eslint

@patricklx
Copy link

patricklx commented Aug 17, 2023

ah, then it needs to be updated in the blueprints themselfs?
if I add this there (in files/package.json)

"eslint-plugin-prettier": "^5.0.0",
    "stylelint-prettier": "^4.0.2",

it passes linting, but this is probably just overwriting the deps from the workspaces

@patricklx
Copy link

patricklx commented Aug 17, 2023

this: https://github.com/ember-cli/ember-cli/blob/master/blueprints/app/files/package.json#L98
but just making guesses now. not sure where that lower version is coming from...

@NullVoxPopuli
Copy link
Collaborator Author

but just making guesses now. not sure where that lower version is coming from...

that's probably accurate -- npm doesn't great dep management -- but then also eslint and prettier both expect their dependencies to be at the root of the monorepo, instead of relying on the actual dep graph, so it makes fixing stuff like this super annoying..

So, we need to get the app and addon blueprints in ember-cli using prettier v3, and then npm will be fixed, probably.

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Aug 17, 2023

this does mean, however, that, if we wait for ember-cli, we have broken GTS for ~ 7 weeks 😅 #184

So I'm not really sure what to do here. 🤔

  • Since we're emitting a monorepo, we could emit overrides as well, but that doesn't feel great.
  • we could also temporarily allow lint failures in the tests

What are folks thoughts?

@patricklx
Copy link

it looks like thats already beeing done for pnpm?
https://github.com/embroider-build/addon-blueprint/blob/main/index.js#L75
can be done for npm as well? or override the version in json.devDependencies directly

@NullVoxPopuli NullVoxPopuli changed the title Upgrade prettier and prettier-plugin-ember-template-tag Blocked(ember-cli): Upgrade prettier and prettier-plugin-ember-template-tag Aug 18, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as draft August 18, 2023 14:17
@NullVoxPopuli NullVoxPopuli changed the title Blocked(ember-cli): Upgrade prettier and prettier-plugin-ember-template-tag Upgrade prettier and prettier-plugin-ember-template-tag Sep 8, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review September 8, 2023 03:01
@NullVoxPopuli NullVoxPopuli merged commit f07c92c into main Sep 8, 2023
15 checks passed
@NullVoxPopuli NullVoxPopuli deleted the upgrade-deps-4 branch September 8, 2023 11:42
chancancode added a commit to discourse/ember-route-template that referenced this pull request Sep 10, 2023
chancancode added a commit to discourse/ember-route-template that referenced this pull request Sep 10, 2023
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.

<template> is broken with prettier in gjs and gts files
4 participants