-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: main
Are you sure you want to change the base?
Conversation
#41 got merged, can you rebase and check what else is needed? |
575e0bc
to
7abb00e
Compare
Copies the `plugin-resolution` tests from prettier.
7abb00e
to
479f692
Compare
@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", () => { |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot details, but tricky part is here https://github.com/prettier/prettier/blob/af6e7215ce0e0d243cb34a85174af65ab4177f47/src/config/resolve-config.js#L67
34e0257
to
f8dc396
Compare
#50 got fixed, do we need more changes to this PR before merging? |
Copies the
plugin-resolution
tests from prettier.Blocked by #41