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

Support Vue #40

Merged
merged 1 commit into from Oct 26, 2022
Merged

Support Vue #40

merged 1 commit into from Oct 26, 2022

Conversation

IanVS
Copy link
Owner

@IanVS IanVS commented Oct 25, 2022

Co-authored-by: Blake Newman <code@blakenewman.dev>
@IanVS IanVS merged commit bd83499 into main Oct 26, 2022
@IanVS IanVS deleted the vue-support branch October 26, 2022 14:38
@@ -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"
Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

optionalDependency and devDependency?

Copy link
Owner Author

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.

Copy link
Collaborator

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.

Copy link
Owner Author

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?

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Owner Author

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.

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.

None yet

3 participants