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

⭐️New: Add rule no-template-shadow. #158

Merged
merged 6 commits into from
Jul 30, 2018

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Aug 14, 2017

fixes #101

@armano2 armano2 changed the title Add rule no-template-shadow. fixes #101 Add rule no-template-shadow. Aug 14, 2017
@michalsnik michalsnik requested a review from mysticatea August 15, 2017 11:00
@michalsnik michalsnik added this to the v4.1.0 milestone Oct 9, 2017
@michalsnik michalsnik changed the title Add rule no-template-shadow. [NEW] Add rule no-template-shadow. Dec 2, 2017
@michalsnik michalsnik changed the title [NEW] Add rule no-template-shadow. [New] Add rule no-template-shadow. Dec 2, 2017
Copy link
Member

@michalsnik michalsnik left a comment

Choose a reason for hiding this comment

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

Good job @armano2 Now we can finally push this forward :) Please update PR when you'll find time


:-1: Examples of **incorrect** code for this rule:

```html
Copy link
Member

Choose a reason for hiding this comment

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

I think we should present two or three cases here. First - a simple template example, and second with shadowed data or method property declarations. e.g.:
1.

<template>
   <div>
     <div v-for="i in 5">
       <div v-for="i in 10"></div>
     </div>
   </div>
 </template>
<template>
   <div>
     <div v-for="i in 5"></div>
   </div>
 </template>
<script>
  export default {
    data () {
      return {
        i: 10
      }
    }
  }
</script>

meta: {
docs: {
description: 'Disallow variable declarations from shadowing variables declared in the outer scope.',
category: 'Possible Errors',
Copy link
Member

Choose a reason for hiding this comment

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

I think it should belong to essential group

@@ -26,7 +26,7 @@ module.exports = {
*
* @param {RuleContext} context The rule context to use parser services.
* @param {Object} templateBodyVisitor The visitor to traverse the template body.
* @param {Object} scriptVisitor The visitor to traverse the script.
* @param {Object} [scriptVisitor] The visitor to traverse the script.
Copy link
Member

Choose a reason for hiding this comment

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

Why square brackets here?

Copy link
Member

Choose a reason for hiding this comment

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

{
filename: 'test.vue',
code: `<template>
<div v-for="i in 5"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Let's add case with i in f, wdyt?

}
</script>`,
errors: [{
message: "Variable 'i' is already declared in the upper scope.",
Copy link
Member

Choose a reason for hiding this comment

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

Please add line on which the following error is expected

@@ -0,0 +1,46 @@
# Disallow variable declarations from shadowing variables declared in the outer scope. (no-template-shadow)

`no-shadow` should report variable definitions of v-for directives or scope attributes if those shadows the variables in parent scopes.
Copy link
Member

Choose a reason for hiding this comment

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

no-template-shadow

@michalsnik michalsnik removed this from the v4.1.0 milestone Jan 6, 2018
@michalsnik michalsnik self-assigned this Mar 22, 2018
@michalsnik michalsnik changed the title [New] Add rule no-template-shadow. ⭐️New: Add rule no-template-shadow. Apr 1, 2018
@michalsnik michalsnik requested review from chrisvfritz and removed request for mysticatea July 30, 2018 11:36
Copy link
Contributor

@chrisvfritz chrisvfritz left a comment

Choose a reason for hiding this comment

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

This looks good to me! Even though there's not currently a rule for this in the style guide, I would place it in strongly-recommended when it's considered stable.

@michalsnik
Copy link
Member

We're still in v5 beta. Let's put it in strongly-recommended so we can get feedback faster, and don't need to wait for the next major release to ship it by default.

@michalsnik michalsnik merged commit da4ea71 into vuejs:master Jul 30, 2018
@armano2 armano2 deleted the 101-no-shadow branch October 3, 2018 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Proposal: vue/no-shadow
4 participants