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

test: migrate plugin-resolution tests #42

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Feb 18, 2025

Copies the plugin-resolution tests from prettier.

Blocked by #41

@fabiospampinato
Copy link
Collaborator

#41 got merged, can you rebase and check what else is needed?

@43081j 43081j force-pushed the test-plugin-resolution branch from 575e0bc to 7abb00e Compare February 23, 2025 17:40
@43081j 43081j force-pushed the test-plugin-resolution branch from 7abb00e to 479f692 Compare February 26, 2025 21:19
@43081j
Copy link
Contributor Author

43081j commented Feb 26, 2025

@fabiospampinato this is blocked by #50


// TODO (43081j): we don't currently support loading relative paths without
// the leading `./`
describe.skip("loads --plugin by filename without leading ./, should resolve to file, not package", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note these skipped tests too

prettier somehow allows foo/bar/baz.js to behave as if it was ./foo/bar/baz.js

no clue how it does this without just trying twice (once for a relative path, once for a node path, because it could also be from the package foo)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aha thank you. so it is what i expected

the question is, do we want this in the new CLI? its easy to add but maybe it'd be a good time to get rid of these relative-but-not-really-relative paths? and just tell people to use actual paths, like ./foo

Copy link
Member

@fisker fisker Feb 26, 2025

Choose a reason for hiding this comment

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

Actually, I like how it works. Imagine if we support ignore files in config.

export default {
  ignore: [
-    "foo.js",
+    "./foo.js"
  ]
}

No body want this.

Same here

export default {
  plugins: [
-    "node_modules/prettier-plugin-foo/index.js",
+    "./node_modules/prettier-plugin-foo/index.js"
  ]
}

much easier to use.

Maybe someday, we should ask user to import plugin like ESLint did.

Copy link
Member

@fisker fisker Feb 26, 2025

Choose a reason for hiding this comment

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

Add the plugin path also not related to config file, but related to CWD, makes no sense to put ./ there.

At least it's how it works now, in v3 I actually try to change it, but turns out it's harder than I thought, so I gave up.

Copy link
Member

Choose a reason for hiding this comment

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

@fabiospampinato fabiospampinato force-pushed the main branch 4 times, most recently from 34e0257 to f8dc396 Compare March 6, 2025 00:45
@fabiospampinato
Copy link
Collaborator

#50 got fixed, do we need more changes to this PR before merging?

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.

3 participants