Add `require-prop-types` rule. #85

armano2 · 2017-07-16T16:06:49Z

This PR implements rules proposed in #19

DONE:

Create documtation
Add tests
Implement rule

mysticatea

Thank you for contributing!

The implementation looks good to me.
But I'm not that this rule name is proper. This rule reports the property definitions which don't have their type definition, so I think the rule name should include "type" word. Maybe require-prop-types?

armano2 · 2017-07-19T22:03:39Z

@mysticatea i like more require-prop-types -> renamed

michalsnik

Few suggestions @armano2, otherwise LGTM 👍

michalsnik · 2017-07-21T09:32:58Z

lib/rules/require-prop-types.js

+  // Helpers
+  // ----------------------------------------------------------------------
+
+  function getPropTypes (componentProperties) {


In my opinion this function contains too much logic. Can you please try to split it into smaller chunks - easier to understand and test? We should make the code as simple as possible, so that it's easier for anyone to contribute later on.
Such functions should preferable contain a small comment above on what they do, what parameters are required and what's the supposed output.

michalsnik · 2017-07-21T09:33:07Z

lib/rules/require-prop-types.js

+        return { key, node: cp, hasType }
+      })
+    } else if (type === 'ArrayExpression') { // props: [
+      // nodes = node.elements


What about this?

michalsnik · 2017-07-21T09:36:56Z

lib/rules/require-prop-types.js

+        if (!cp.hasType) {
+          context.report({
+            node: cp.node || data.node,
+            message: 'Prop "{{name}}" definitions should always be as detailed with at least type(s).',


What about this message instead?
Prop "{{name}}" should define at least it's type.

michalsnik · 2017-07-21T09:38:52Z

lib/rules/require-prop-types.js

+    } else if (data.type === 'ArrayExpression') {
+      context.report({
+        node: data.node,
+        message: 'Props definitions should always be an object and detailed with at least type(s).'


Maybe this instead?
Props' should at least define their types.
or
Prop types are required

armano2 · 2017-07-21T19:05:34Z

@michalsnik suggestions applied 🐶

michalsnik

Almost there 🚀

michalsnik · 2017-07-22T11:45:39Z

lib/rules/require-prop-types.js

+  // ----------------------------------------------------------------------
+
+  function objectHasType (nodes) {
+    return !!(nodes


KISS:

const typeProperty = nodes.filter(p => // ... return Boolean(typeProperty)

michalsnik · 2017-07-22T11:48:20Z

lib/rules/require-prop-types.js

+      const key = cp.key.name
+      let hasType = true
+      if (cp.value.type === 'ObjectExpression') { // foo: {
+        hasType = objectHasType(cp.value.properties)


If this function name is to be accurate you should pass cp.value as it's an ObjectExpression.

armano2 · 2017-07-22T12:55:03Z

@michalsnik i found out that we are not supporting

['type']: ....

in rules 🗡 and i added getStaticPropertyName what we should always use when we are trying to get name of object property name

michalsnik · 2017-07-23T12:50:59Z

lib/utils/index.js

+   * Gets the property name of a given node.
+   * @param {ASTNode} node - The node to get.
+   * @return {string|null} The property name if static. Otherwise, null.
+   */


Please add unit tests for this method, I just realised they're not there.

@michalsnik added

It's sweet memories, I wrote it at eslint/lib/ast-utils.
I hope eslint/lib/ast-utils to separate to another package (though it's an item of ESLint 5 wish list).

this method is not same but almost, and ast-utils is not exported than i will have to import file directly what can cause some errors between versions of eslint.

mysticatea

Thank you! Mostly LGTM. There are some nit-picking.

mysticatea · 2017-07-23T16:05:48Z

lib/rules/require-prop-types.js

+
+  return utils.executeOnVue(context, (obj) => {
+    const node = obj.properties
+      .filter(p =>


We can use Array#find instead.

http://node.green/#ES2015-built-ins-typed-arrays--TypedArray--prototype-find

mysticatea · 2017-07-23T16:07:27Z

lib/rules/require-prop-types.js

+  }
+
+  function checkProperties (items) {
+    items.map(cp => {


I'm wondering that it uses Array#map to iterate the array. I like for-of 😃

this map is incorrect 👍 we are even not returning any values 🍡

mysticatea · 2017-07-23T16:08:00Z

lib/rules/require-prop-types.js

+
+  function objectHasType (node) {
+    const typeProperty = node.properties
+      .filter(p =>


We can use Array#find instead.

i have to start using http://node.green/ to be sure what i can/can't use 🍏

armano2 · 2017-07-23T16:35:25Z

@mysticatea suggestions applied, thank you, and thank you for sharing "http://node.green/"

fixes vuejs#19

mysticatea

LGTM, thank you for amazing work!

* master: Add rule `vue/require-valid-default-prop`. (vuejs#119) 3.10.0 Update readme to 3.10.0 Chore: remove package-lock.json (vuejs#128) Fix: parserService must exist always (fixes vuejs#125) (vuejs#127) Add rule `require-render-return`. (vuejs#114) 3.9.0 Update package-lock Update: options for `no-duplicate-attributes` (fixes vuejs#112)(vuejs#113) New: add rule `attribute-hyphenation`. (fixes vuejs#92)(vuejs#95) Add namespace check of svg & mathML instead of tag names (vuejs#120) ⚠️ Add support for deprecated state in update-rules ⚠️ (vuejs#121) Add rules: `no-dupe-keys` and `no-reserved-keys`. (vuejs#88) Chore: Improve tests for name-property-casing & improve documentation (vuejs#115) New: add `require-prop-types` rule (fixes vuejs#19)(vuejs#85) Update: upgrade vue-eslint-parser (fixes vuejs#36, fixes vuejs#56, fixes vuejs#96) (vuejs#116)

michalsnik added work in progress do not merge labels Jul 16, 2017

armano2 changed the title ~~WIP: Add prop-specificity rule.~~ Add prop-specificity rule. Jul 17, 2017

armano2 force-pushed the patch-10-prop-specificity branch from b53a8e0 to 71e583e Compare July 19, 2017 17:39

mysticatea added Ready for review semver-minor and removed do not merge work in progress labels Jul 19, 2017

mysticatea suggested changes Jul 19, 2017

View reviewed changes

armano2 changed the title ~~Add prop-specificity rule.~~ Add require-prop-types rule. Jul 19, 2017

michalsnik reviewed Jul 21, 2017

View reviewed changes

michalsnik suggested changes Jul 22, 2017

View reviewed changes

armano2 force-pushed the patch-10-prop-specificity branch from f6f4872 to c53fa49 Compare July 22, 2017 16:45

michalsnik approved these changes Jul 23, 2017

View reviewed changes

michalsnik suggested changes Jul 23, 2017

View reviewed changes

armano2 force-pushed the patch-10-prop-specificity branch 2 times, most recently from 7e19002 to 2dc3d98 Compare July 23, 2017 13:52

mysticatea reviewed Jul 23, 2017

View reviewed changes

michalsnik approved these changes Jul 24, 2017

View reviewed changes

Add require-prop-types rule.

d7d5182

fixes vuejs#19

armano2 force-pushed the patch-10-prop-specificity branch from cd86f33 to d7d5182 Compare July 31, 2017 19:50

mysticatea approved these changes Aug 1, 2017

View reviewed changes

mysticatea merged commit b8d11de into vuejs:master Aug 1, 2017

armano2 deleted the patch-10-prop-specificity branch August 1, 2017 13:33

Uh oh!

Add require-prop-types rule. #85

Add require-prop-types rule. #85

Uh oh!

Conversation

armano2 commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mysticatea left a comment

Choose a reason for hiding this comment

Uh oh!

armano2 commented Jul 19, 2017

Uh oh!

michalsnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armano2 commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalsnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armano2 commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalsnik Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armano2 Jul 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mysticatea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armano2 commented Jul 23, 2017

Uh oh!

mysticatea left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `require-prop-types` rule. #85

Add `require-prop-types` rule. #85

armano2 commented Jul 16, 2017 •

edited

Loading

armano2 commented Jul 21, 2017 •

edited

Loading

armano2 commented Jul 22, 2017 •

edited

Loading

michalsnik Jul 23, 2017 •

edited

Loading

armano2 Jul 23, 2017 •

edited

Loading