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

fix(vue): resolve src attribute on the script block on Vue files #130

Merged
merged 6 commits into from Jul 26, 2018

Conversation

Projects
None yet
6 participants
@ktsn
Contributor

ktsn commented Jul 25, 2018

This PR fixes #111 and vuejs/vue-cli#1104. Also probably fixes #85.

I've replaced vue-parser with vue-template-compiler which is Vue official package for parsing .vue file. Because vue-parser looks not maintained and it would be better to use officially provided lib for consistency.

I didn't include vue-template-compiler in dependencies because there is a known issue that mismatching with user-installed vue version.
The users should not feel inconvenience by this because vue-loader (official webpack loader for .vue file) enforces the users to install vue-template-compiler due to this issue. That means if the users want to use vue option of this plugin, they already install vue-template-compiler by themselves.

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jul 25, 2018

Collaborator

Thanks for the PR @ktsn! Looks initially good to me.

@prograhammer @CKGrafico @wonderful-panda I believe you're Vue users and have worked on the Vue support in the plugin. Would you be able to look at this PR and let us know what you think?

cc @yyx990803

Collaborator

johnnyreilly commented Jul 25, 2018

Thanks for the PR @ktsn! Looks initially good to me.

@prograhammer @CKGrafico @wonderful-panda I believe you're Vue users and have worked on the Vue support in the plugin. Would you be able to look at this PR and let us know what you think?

cc @yyx990803

@CKGrafico

This comment has been minimized.

Show comment
Hide comment
@CKGrafico

CKGrafico Jul 25, 2018

Contributor

Looks really nice!

Contributor

CKGrafico commented Jul 25, 2018

Looks really nice!

@wonderful-panda

This comment has been minimized.

Show comment
Hide comment
@wonderful-panda

wonderful-panda Jul 25, 2018

Contributor

LGTM

Contributor

wonderful-panda commented Jul 25, 2018

LGTM

@yyx990803

This comment has been minimized.

Show comment
Hide comment
@yyx990803

yyx990803 commented Jul 26, 2018

👍

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jul 26, 2018

Collaborator

Okay - I think we're good to merge! @ktsn can you comment on whether the vue-template-compiler types are directly consumable or not?

Collaborator

johnnyreilly commented Jul 26, 2018

Okay - I think we're good to merge! @ktsn can you comment on whether the vue-template-compiler types are directly consumable or not?

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Jul 26, 2018

Contributor

Thanks for taking a look at this PR 🙏

@johnnyreilly The current vue-template-compiler does not include the types but it would be included and we will be able to consume it directly in the future since there is a PR already (maybe vue-template-compiler v2.6.0?).

Contributor

ktsn commented Jul 26, 2018

Thanks for taking a look at this PR 🙏

@johnnyreilly The current vue-template-compiler does not include the types but it would be included and we will be able to consume it directly in the future since there is a PR already (maybe vue-template-compiler v2.6.0?).

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jul 26, 2018

Collaborator

Awesome! Then let's go with this for now and if that PR gets merged we can switch to consuming from there later. Thanks for your work!

Collaborator

johnnyreilly commented Jul 26, 2018

Awesome! Then let's go with this for now and if that PR gets merged we can switch to consuming from there later. Thanks for your work!

@johnnyreilly johnnyreilly merged commit 6a83c7d into Realytics:master Jul 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ktsn ktsn deleted the ktsn:fix-vue-src branch Jul 26, 2018

@johnnyreilly

This comment has been minimized.

Show comment
Hide comment
@johnnyreilly

johnnyreilly Jul 26, 2018

Collaborator

Merged - this will be published once a new token has been generated for publishing. See #132

Collaborator

johnnyreilly commented Jul 26, 2018

Merged - this will be published once a new token has been generated for publishing. See #132

@nobelhuang

This comment has been minimized.

Show comment
Hide comment
@nobelhuang

nobelhuang Aug 7, 2018

I'm not pretty sure, but seems this is the change that will somewhat cause tslint to complain if no-consecutive-blank-lines rule is used when we try to compile those vue files written in TS/TSX. I guess it is just because vue-template-compiler didn't remove those blank lines after extracting ts part from SFC, then we got this error:

no-consecutive-blank-lines: Consecutive blank lines are forbidden

nobelhuang commented Aug 7, 2018

I'm not pretty sure, but seems this is the change that will somewhat cause tslint to complain if no-consecutive-blank-lines rule is used when we try to compile those vue files written in TS/TSX. I guess it is just because vue-template-compiler didn't remove those blank lines after extracting ts part from SFC, then we got this error:

no-consecutive-blank-lines: Consecutive blank lines are forbidden

@ktsn

This comment has been minimized.

Show comment
Hide comment
@ktsn

ktsn Aug 8, 2018

Contributor

We should not trim blank line because reported diagnostics will point to wrong location if we do that.

I remember vue-template-compiler pads the blank lines with // to avoid such lint errors (almost the same approach with vue-parser) but it looks applicable only when <script> block lang is JavaScript.

I will make a PR to fix the issue later.

Contributor

ktsn commented Aug 8, 2018

We should not trim blank line because reported diagnostics will point to wrong location if we do that.

I remember vue-template-compiler pads the blank lines with // to avoid such lint errors (almost the same approach with vue-parser) but it looks applicable only when <script> block lang is JavaScript.

I will make a PR to fix the issue later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment