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

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

Merged
merged 10 commits into from
Dec 22, 2018

Conversation

gzzhanghao
Copy link
Contributor

@gzzhanghao 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
Copy link
Member

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.

Copy link
Member

@HerringtonDarkholme HerringtonDarkholme left a comment

Choose a reason for hiding this comment

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

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'))

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}

export function getRawBindingAttr (

Choose a reason for hiding this comment

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

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 (

Choose a reason for hiding this comment

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

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--) {

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@gzzhanghao
Copy link
Contributor Author

@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
Copy link
Member

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

@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 December 21, 2018 19:16
@yyx990803 yyx990803 merged commit b31a1aa into vuejs:2.6 Dec 22, 2018
2.6 automation moved this from In progress to Done Dec 22, 2018
@lbogdan
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
Copy link
Member

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

@lbogdan
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
@lusarz
Copy link

lusarz commented May 31, 2019

@yyx990803 I need to tranfsorm start, end to be line number. I'm writing small tool to static analysis of code. I have something like:

const content = FileUtils.readFile(path);
const template = compiler.parseComponent(content).template.content;
const ast = compiler.compile(template, { outputSourceRange: true }).ast;

I'm searching for "something" in ast. When I find "something" I'd like to create output like f.e.:

/components/v-some-component.vue:11

where 11 is line number. Do you you think I should do it myself in my code or I can create issue to ask about extending CompilerOptions ?

For example scss-parser create output for start or end of node like { "cursor": 60, "line": 3, "column": 0 }.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants