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.

Sign up for GitHub

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

fix: prettier not working for svelte files in create-svelte projects #6866

Merged
merged 3 commits into from
Sep 19, 2022
Merged

fix: prettier not working for svelte files in create-svelte projects #6866

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4104aab
fix: prettier not working for svelte files in create-svelte projects
dominikg Sep 17, 2022
9863748
fix: format
dominikg Sep 17, 2022
d55a8bf
changeset format
dummdidumm Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/light-eagles-call.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'create-svelte': patch
---

fix prettier not formatting svelte files
4 changes: 2 additions & 2 deletions packages/create-svelte/shared/+eslint+prettier/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"eslint-config-prettier": "^8.3.0"
},
"scripts": {
"lint": "prettier --check . && eslint .",
"format": "prettier --write ."
"lint": "prettier --plugin-search-dir . --check . && eslint .",
"format": "prettier --plugin-search-dir . --write ."
}
}
1 change: 1 addition & 0 deletions packages/create-svelte/shared/+prettier/.prettierrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"singleQuote": true,
"trailingComma": "none",
"printWidth": 100,
"plugins": ["prettier-plugin-svelte"],
"pluginSearchDirs": ["."],
Comment on lines +6 to 7
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should do

Suggested change
"plugins": ["prettier-plugin-svelte"],
"pluginSearchDirs": ["."],
"plugins": ["./node_modules/prettier-plugin-svelte"],

instead? I'm using this hack for all my projects which works well when running prettier through the CLI or VSCode extension. The --plugin-search-dir . can also be removed too.

I made a comment at sveltejs/prettier-plugin-svelte#155 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

would that work with yarn? also the search dir setting is likely needed if the users add more prettier plugins.

Copy link
Member

@dummdidumm dummdidumm Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just from reading this I'm not sure if the requested change by @bluwy is the same as the proposal, or works for cases where the proposed changes by @dominikg don't work?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah my suggestion doesn't seem to work with yarn pnp (pnp also messes up vscode prettier plugin with this PR change and before). I guess we can go with this PR change still to have it partially working for yarn pnp.

also the search dir setting is likely needed if the users add more prettier plugins.

Since they'd have to still add them in plugins with this PR, e.g. "plugins": ["prettier-plugin-svelte", "prettier-plugin-something"], I don't think the plugin-search-dir would make a difference, besides not needing the ./node_modules/ part of my suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

i prefer not having node_modules listed explicitly. I'd love to get rid of the --plugin-search-dir . on the cli though so it's a tough one. Ultimately this PR uses multiple ways to try and ensure the plugin is found. The only other option i know would be to use prettierrc.cjs and require('prettier-plugin-svelte') to take resolving out of prettiers own hands. But thats also ugly.

I think the way this PR uses both json rc and cli option to ensure the plugin is found during api and cli use is working and has only one duplication, which is unfortunate but not that bad.

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with this solution then

"overrides": [{ "files": "*.svelte", "options": { "parser": "svelte" } }]
}
4 changes: 2 additions & 2 deletions packages/create-svelte/shared/-eslint+prettier/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"scripts": {
"lint": "prettier --check .",
"format": "prettier --write ."
"lint": "prettier --plugin-search-dir . --check .",
"format": "prettier --plugin-search-dir . --write ."
}
}
15 changes: 12 additions & 3 deletions packages/create-svelte/test/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import fs from 'fs';
import { execSync } from 'child_process';
import path from 'path';
import { test } from 'uvu';
import { Writable } from 'stream';
import * as assert from 'uvu/assert';
import { create } from '../index.js';
import { fileURLToPath } from 'url';
Expand All @@ -25,6 +24,16 @@ const overrides = { ...existing_workspace_overrides };
overrides[name] = path.dirname(path.resolve(pkgPath));
});

try {
const kit_dir = fileURLToPath(new URL('../../../packages/kit', import.meta.url));
const ls_vite_result = execSync(`pnpm ls --json vite`, { cwd: kit_dir });
const vite_version = JSON.parse(ls_vite_result)[0].devDependencies.vite.version;
overrides.vite = vite_version;
} catch (e) {
console.error('failed to parse installed vite version from packages/kit');
throw e;
}

test.before(() => {
try {
// prepare test pnpm workspace
Expand Down Expand Up @@ -90,7 +99,7 @@ for (const template of fs.readdirSync('templates')) {
execSync('pnpm install --no-frozen-lockfile', { cwd, stdio: 'ignore' });

// run provided scripts that are non-blocking. All of them should exit with 0
const scripts_to_test = ['prepare', 'check', 'lint', 'build', 'sync'];
const scripts_to_test = ['sync', 'format', 'lint', 'check', 'build'];

// package script requires lib dir
if (fs.existsSync(path.join(cwd, 'src', 'lib'))) {
Expand All @@ -99,7 +108,7 @@ for (const template of fs.readdirSync('templates')) {

// not all templates have all scripts
console.group(`${template}-${types}`);
for (const script of Object.keys(pkg.scripts).filter((s) => scripts_to_test.includes(s))) {
for (const script of scripts_to_test.filter((s) => !!pkg.scripts[s])) {
try {
execSync(`pnpm run ${script}`, { cwd, stdio: 'pipe' });
console.log(`✅ ${script}`);
Expand Down