-
Notifications
You must be signed in to change notification settings - Fork 24
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
Support Vue #40
Support Vue #40
Conversation
Co-authored-by: Blake Newman <[email protected]>
@@ -52,7 +52,8 @@ | |||
"@babel/types": "^7.17.0", | |||
"javascript-natural-sort": "0.7.1", | |||
"lodash.clone": "^4.5.0", | |||
"lodash.isequal": "^4.5.0" | |||
"lodash.isequal": "^4.5.0", | |||
"@vue/compiler-sfc": "3.2.40" |
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.
Should this have instead been an optionalDependency
? I admit I'm unfamiliar with the size/complexity of @vue/compiler-sfc
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.
optionalDependency
and devDependency
?
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.
I don't think it can be optional, unless we have a good way to conditionally import()
it, and I'm not sure how we'd know up-front when registering the plugin whether it will be needed or not.
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.
try {
var foo = require('foo')
var fooVersion = require('foo/package.json').version
} catch (er) {
foo = null
}
if ( notGoodFooVersion(fooVersion) ) {
foo = null
}
Conditional import will require some shenanigans with require statements, but I think it's possible.
The way I'd do it, is by adding a wrapper function around the compiler call, that does that detection, and throws a clean error if the package is missing, but that codepath is executed. We would need some // @ts-ignore
lines, I think.
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.
I think it would need to be in peerDependencies
and we would set peerDependenciesMeta
to treat it as optional. The main thing I'm not sure about is: Can we expect every Vue project to have this dependency?
@blake-newman, since you wrote the original PR and know the Vue landscape much better than I do, maybe you can talk about why you chose to use a regular dependency for @vue/compiler-sfc
rather than a peerDependency?
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.
@IanVS i used it as a devDependency
for simplicity of use; however it is correct that not all will need it.
This is the stack it introduces: which is fairly heavy just to extract the script section. I may be able to remove this dependency tbh. As it's not a tricky job to do so.
"@vue/compiler-sfc@npm:^3.2.40":
version: 3.2.41
resolution: "@vue/compiler-sfc@npm:3.2.41"
dependencies:
"@babel/parser": ^7.16.4
"@vue/compiler-core": 3.2.41
"@vue/compiler-dom": 3.2.41
"@vue/compiler-ssr": 3.2.41
"@vue/reactivity-transform": 3.2.41
"@vue/shared": 3.2.41
estree-walker: ^2.0.2
magic-string: ^0.25.7
postcss: ^8.1.10
source-map: ^0.6.1
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.
trivago/prettier-plugin-sort-imports#183
I decided to keep with the compiler; as no point re-inventing the wheel. Most Vue projects will have this dependency by default anyway.
The scope of the API used is very minimal so there is no need to try to lock it into a specific version; it's quite unlikely the API will break this use case. If it ever does it'll be likely trivial changes to fix but won't break this package and fix can be added consumer side to lock a particular version until resolved upstream.
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.
Great, thanks for the explanations! I'll make it a peer dependency on this fork as well, then.
Also note, if you're using the trivago version, it's potentially unsafe to use side-effect-only imports, since it will re-arrange them, which is the reason this fork exists. Just giving you a heads-up in case you weren't aware.
Ported from trivago/prettier-plugin-sort-imports#174