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

feat(compiler): output source range for compiler errors (#6338) #7127

Merged
merged 10 commits into from Dec 22, 2018

Conversation

Projects
4 participants
@gzzhanghao
Copy link
Contributor

gzzhanghao commented Nov 25, 2017

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

Just like #6764, but will output the range data instead of writing it into the error message. Here's an example of how it works:

https://gist.github.com/gzzhanghao/363fac4a201306f5fccf903e35bc652a

@yyx990803

This comment has been minimized.

Copy link
Member

yyx990803 commented Nov 27, 2017

This is great! Thanks a lot for the work. Since this is a big PR, it will take some time to properly review. But I hope we can include this in 2.6.

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme left a comment

Thanks for your contribution!
I have seen that most warning code is enclosed in conditional env block. But it would be better if you can report the new bundle size in the PR.
Great work!


export default function on (el: ASTElement, dir: ASTDirective) {
if (process.env.NODE_ENV !== 'production' && dir.modifiers) {
warn(`v-on without argument does not support modifiers.`)
warn(`v-on without argument does not support modifiers.`, getRawAttr(el, 'v-on'))

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Nov 28, 2017

Member

I might be wrong, but warn's second parameter seems to accept Vue vm instance, but getRawAttr returns AstAttr

This comment has been minimized.

@gzzhanghao

gzzhanghao Nov 28, 2017

Author Contributor

You're right, I didn't notice that warn is different from the one in other files.

}
}

export function getRawBindingAttr (

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Nov 28, 2017

Member

Can you add a test for getRawBindingAttr, I mean, <div v-bind:key="wrong-key"> should return an error with :key's range, even if we call getRawBindingAttr(el, 'key').

@@ -149,3 +181,18 @@ export function getAndRemoveAttr (
}
return val
}

function setRange (

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Nov 28, 2017

Member

A bit nit-picky, but setRange implies side-effect, rangeSetItem might be a better naming.

@@ -22,6 +22,13 @@ function transformNode (el: ASTElement, options: CompilerOptions) {
el.attrsMap[realName] = el.attrsMap[attr.name]
delete el.attrsMap[attr.name]
}
const rawAttrsList = el.rawAttrsList || []
for (let i = rawAttrsList.length - 1; i >= 0; i--) {

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Nov 28, 2017

Member

This is a quadratic time complexity. I wonder if it will impact compile time.

This comment has been minimized.

@gzzhanghao

gzzhanghao Nov 28, 2017

Author Contributor

Switched to using an object to track raw attribute info in 5991522

@gzzhanghao gzzhanghao force-pushed the gzzhanghao:dev branch from 678f2f6 to b474d24 Nov 28, 2017

@gzzhanghao gzzhanghao force-pushed the gzzhanghao:dev branch from b474d24 to ca71c0d Nov 28, 2017

@gzzhanghao gzzhanghao force-pushed the gzzhanghao:dev branch from 797cb85 to 5991522 Nov 28, 2017

@gzzhanghao

This comment has been minimized.

Copy link
Contributor Author

gzzhanghao commented Nov 28, 2017

@HerringtonDarkholme I've just enclosed outputSourceRange code into conditional env block in facacc1

And here are the new bundle size from npm run build:

File                                          | Origin        | New
---
dist\vue.runtime.common.js                    | 220.81        | 220.82
dist\vue.common.js                            | 318.01        | 322.35
dist\vue.esm.js                               | 318.00        | 322.33
dist\vue.js                                   | 315.06        | 319.35
dist\vue.min.js                               | 91.54 (31.78) | 92.21 (32.00)
packages\vue-template-compiler\build.js       | 177.66        | 185.11
packages\vue-template-compiler\browser.js     | 260.55        | 267.90
packages\vue-server-renderer\build.js         | 261.09        | 265.02
packages\vue-server-renderer\basic.js         | 316.17        | 320.05
@HerringtonDarkholme

This comment has been minimized.

Copy link
Member

HerringtonDarkholme commented Nov 28, 2017

@gzzhanghao Thanks! Size overhead seems to be acceptable! Thanks again!

@gzzhanghao gzzhanghao force-pushed the gzzhanghao:dev branch from 40da325 to e493d79 Dec 13, 2017

@yyx990803 yyx990803 added this to Todo in 2.6 Dec 5, 2018

@yyx990803 yyx990803 moved this from Todo to In progress in 2.6 Dec 20, 2018

@yyx990803 yyx990803 moved this from In progress to Todo in 2.6 Dec 20, 2018

@yyx990803 yyx990803 moved this from Todo to In progress in 2.6 Dec 21, 2018

@yyx990803 yyx990803 changed the base branch from dev to 2.6 Dec 21, 2018

@yyx990803 yyx990803 merged commit b31a1aa into vuejs:2.6 Dec 22, 2018

5 checks passed

ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint-flow-types Your tests passed on CircleCI!
Details
ci/circleci: test-cover Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-ssr-weex Your tests passed on CircleCI!
Details

2.6 automation moved this from In progress to Done Dec 22, 2018

@lbogdan

This comment has been minimized.

Copy link
Contributor

lbogdan commented Jan 16, 2019

So nice to see this land! Do you know of any effort implementing support for this in vue-loader? If not, I could maybe tackle it?

@yyx990803

This comment has been minimized.

Copy link
Member

yyx990803 commented Jan 16, 2019

@lbogdan it already does! (not released yet). But thanks for offering to help!

@lbogdan

This comment has been minimized.

Copy link
Contributor

lbogdan commented Jan 16, 2019

Oh, nice, for some reason I was looking at PRs, and there was nothing there. 🙂
Played with it (your version) a bit, while the output is nice, it tends to get confusing with big templates (because, for example, the below error doesn't have an end marker, so generateCodeFrame() will return the whole template starting from start).

I was thinking maybe something like
might be a bit more useful, as you can see the line, and also you can Ctrl+click in VSCode and it gets you to that exact location.

f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019

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