Allow to use @vue/component to set enable parsing vue objects #123

armano2 · 2017-08-02T15:33:16Z

fixes #109

michalsnik · 2017-08-03T23:07:57Z

tests/lib/rules/no-dupe-keys.js

@@ -77,8 +77,8 @@ ruleTester.run('no-dupe-keys', rule, {

  invalid: [
    {
-      filename: 'test.vue',
-      code: `
+      // filename: 'test.vue',


Please create another tests for that, don't comment actual ones. Also try case with block comment too :)

armano2 · 2017-08-04T17:21:22Z

@michalsnik new tests added

michalsnik

LGTM 👍 Nice one. Only one more thing - we need to add information about this somewhere in readme, so that people actually know it's there and that it's ready to be used.

armano2 · 2017-08-04T20:47:41Z

@michalsnik do you have any suggestions? I'm terrible at describing/naming stuff xd

michalsnik · 2017-08-04T21:13:52Z

I was thinking about adding information in Usage section, that we currently detect components based on certain requirements (list them) and if someone wants custom .js files to be checked in terms of the available rules he/she needs to add special comment, so that we can assume it's also to be considered as a component.

Btw. Now I realised that there might be other cases, when you for example want to create few small components inside one .js files and compose them, you could get:

// utils/example.js
const ChildComponent = {
  template: `<div>{{msg}}</div>`,
  data () {
    return { msg: 'Hello child' }
  }
}

export default {
 template: `<child-component />`,
  components: {
    ChildComponent
  }
}

such a situation might also have place in .vue files. My proposition would be to detect if @vue/component is exactly one line above the ObjectExpression - if yes -> treat the ObjectExpression as a component. This should solve all cases and shouldn't be hard to implement :) What do you think @armano2 ?

The above file could look like this:

// @vue/component
const ChildComponent = {
  template: `<div>{{msg}}</div>`,
  data () {
    return { msg: 'Hello child' }
  }
}

// @vue/component
export default {
 template: `<child-component />`,
  components: {
    ChildComponent
  }
}

armano2 · 2017-08-04T21:27:48Z

@michalsnik hmm i like it, and i was thinking about it before.

component definition should work only if next block is
export default {
export const/var/let [name] = {
const/var/let [name] = {

what do you think?

michalsnik · 2017-08-04T21:35:42Z

There are also async components, I wonder if simple check for an object right below comment wouldn't be enough. Then it would be also possible to do:

Vue.component('async-example', function (resolve, reject) {
  setTimeout(function () {
    // @vue/component
    resolve({
      template: '<div>I am async!</div>'
    })
  }, 1000)
})

armano2 · 2017-08-04T21:38:32Z

@michalsnik i'm going to create separate test file for all cases to have them all in one place and i'm going to include async component to

armano2 · 2017-08-04T22:02:28Z

@michalsnik can you check if i didn't forgo about something?

https://github.com/armano2/eslint-plugin-vue/blob/patch-28-comment-vue-component/tests/lib/utils/vue-component.js

michalsnik · 2017-08-04T22:26:15Z

tests/lib/utils/vue-component.js

+    },
+    {
+      filename: 'test.js',
+      code: `Vue.component('async-example', function (resolve, reject) { })`,


resolve({}) missing

michalsnik · 2017-08-04T22:26:54Z

tests/lib/utils/vue-component.js

+      parserOptions
+    },
+    {
+      filename: 'test.js',


Add some cases with .jsx and .vue files too that are not to be treated as components by default.

michalsnik · 2017-08-04T22:28:29Z

tests/lib/utils/vue-component.js

+      errors: [makeError(1)]
+    },
+    {
+      filename: 'test.vue',


Add the same test for test.jsx file

michalsnik · 2017-08-04T22:29:15Z

tests/lib/utils/vue-component.js

+    {
+      filename: 'test.js',
+      code: `Vue.component('async-example', function (resolve, reject) {
+        resolve({})


This should actually be not treated as component, we're not supporting this case by default, only with optional comment.

armano2 · 2017-08-05T20:22:59Z

@michalsnik i applied all requested changes, and all tests are more generic now.

michalsnik

Added few suggestions and questions. Plus we still don't have an information in readme about those comments.

michalsnik · 2017-08-05T21:14:46Z

tests/lib/utils/vue-component.js

+      code: `export default { }`,
+      parserOptions
+    }
+  ].concat(notFoundTests('js')).concat(notFoundTests('jsx')).concat(notFoundTests('vue')),


I'd call it validTests and invalidTests for consistency.

michalsnik · 2017-08-05T21:17:44Z

tests/lib/utils/vue-component.js

+  return [
+    {
+      filename: `test.${ext}`,
+      code: `// ${ext}


I guess you added those comments for easier debugging?
If yes please keep them in once place, as for now I can see them on top and bottom of the code example.

Yes that was the point, and i moved them to down (i forgot to this will all of them)

michalsnik · 2017-08-05T21:20:26Z

tests/lib/utils/vue-component.js

+    return utils.executeOnVueComponent(context, obj => {
+      context.report({
+        node: obj,
+        message: 'Internal test: component.'


I think Component detected message would be a little bit more descriptive.

@michalsnik changed

michalsnik · 2017-08-05T21:24:49Z

lib/utils/index.js

      'ExportDefaultDeclaration:exit' (node) {
        // export default {} in .vue || .jsx
-        if (!_this.isVueComponentFile(node, filePath)) return
+        if (!_this.isVueComponentFile(node, filePath, context) || hasNode(node)) return


Why did you passed context to isVueComponentFile? And why did you added hasNode here? I thought current check is ok and we only needed to add comment detection before objects?

context removed and to prevent duplicates from callback, i renamed this function to isDuplicateNode

michalsnik · 2017-08-05T21:25:19Z

lib/utils/index.js

        cb(node.declaration)
      },
      'CallExpression:exit' (node) {
        // Vue.component('xxx', {}) || component('xxx', {})
-        if (!_this.isVueComponent(node)) return
+        if (!_this.isVueComponent(node) || foundNodes.some(el => el.loc.start.line === node.loc.start.line)) return


Why did you added here additional check too?

to prevent duplicates when someone do callExpression and declare // @vue/component

Ah I see, good thinking :)

michalsnik · 2017-08-05T21:27:18Z

tests/lib/utils/vue-component.js

+    },
+    {
+      filename: `test.${ext}`,
+      code: `// ${ext}


Please consider formatting those code example as follows:

code: ` /* @vue/component */ export default {} `

I think they would be much easier to read :)

armano2 · 2017-08-05T22:06:08Z

@michalsnik thank you for suggestions, changes applied

michalsnik · 2017-08-05T22:21:10Z

Cool @armano2 Now please add info in readme and we're good to go :) Good work!

armano2 · 2017-08-06T14:59:12Z

@michalsnik now the hardest part 👎

i wrote something, but i don't like it :(

michalsnik · 2017-08-06T15:28:51Z

@armano2 And what do you think about:

Attention

All component-related rules are being applied to code that passes any of the following checks:

Vue.component() expression
export default {} in .vue or .jsx file

If you however want to take advantage of our rules in any of your custom objects that are Vue components, you might need to use special comment // @vue/component that marks object in the next line as a Vue component in any file, e.g.:

// @vue/component
const CustomComponent = {
  name: 'custom-component',
  template: '<div></div>'
}

Vue.component('AsyncComponent', (resolve, reject) => {
  setTimeout(() => {
    // @vue/component
    resolve({
      name: 'async-component',
      template: '<div></div>'
    })
  }, 500)
})

armano2 · 2017-08-06T15:33:56Z

@michalsnik i like it way more

i'm just terrible at writing documentations

michalsnik

LGTM 👍 But can you please also take a look @mysticatea ?

fixes vuejs#109

armano2 · 2017-08-06T21:19:04Z

@michalsnik There is one issue with this solution, it will not work with eslint 4

https://github.com/eslint/eslint/blob/8796d55a75a2d54b384c2ce3249861d5fe127b12/docs/user-guide/migrating-to-4.0.0.md#-linecomment-and-blockcomment-events-will-no-longer-be-emitted-during-ast-traversal

LineComment and BlockComment was removed in 4.0.0

michalsnik suggested changes Aug 3, 2017

View reviewed changes

michalsnik added the semver-minor label Aug 3, 2017

armano2 force-pushed the patch-28-comment-vue-component branch from 5ff441f to 56a0306 Compare August 4, 2017 17:02

armano2 force-pushed the patch-28-comment-vue-component branch from c3484a4 to eacc0d0 Compare August 4, 2017 17:22

michalsnik reviewed Aug 4, 2017

View reviewed changes

michalsnik suggested changes Aug 4, 2017

View reviewed changes

armano2 force-pushed the patch-28-comment-vue-component branch from dad5cd6 to 753d86d Compare August 5, 2017 01:25

michalsnik suggested changes Aug 5, 2017

View reviewed changes

michalsnik approved these changes Aug 6, 2017

View reviewed changes

armano2 force-pushed the patch-28-comment-vue-component branch from db720cf to 93ca30f Compare August 6, 2017 20:40

armano2 changed the title ~~Allow to use @vue/component to set type of file to vue~~ Allow to use @vue/component to set enable parsing vue objects Aug 6, 2017

Allow to use @vue/component to set enable parsing vue objects

b7b4991

fixes vuejs#109

armano2 force-pushed the patch-28-comment-vue-component branch from 93ca30f to b7b4991 Compare August 6, 2017 20:43

armano2 added 2 commits August 6, 2017 23:49

Add support for eslint 4.x

9db6569

LineComment and BlockComment was removed in 4.0.0

Remove unessesery function

ba38e5a

armano2 force-pushed the patch-28-comment-vue-component branch from 1646194 to ba38e5a Compare August 6, 2017 22:14

michalsnik merged commit 4d0c1d3 into vuejs:master Aug 8, 2017

armano2 deleted the patch-28-comment-vue-component branch August 8, 2017 15:11

Allow to use @vue/component to set enable parsing vue objects #123

Allow to use @vue/component to set enable parsing vue objects #123

Conversation

armano2 commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 commented Aug 4, 2017

michalsnik left a comment • edited Loading

Choose a reason for hiding this comment

armano2 commented Aug 4, 2017 • edited Loading

michalsnik commented Aug 4, 2017 • edited Loading

armano2 commented Aug 4, 2017 • edited Loading

michalsnik commented Aug 4, 2017

armano2 commented Aug 4, 2017

armano2 commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 commented Aug 5, 2017 • edited Loading

michalsnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armano2 commented Aug 5, 2017

michalsnik commented Aug 5, 2017

armano2 commented Aug 6, 2017

michalsnik commented Aug 6, 2017

Attention

armano2 commented Aug 6, 2017

michalsnik left a comment

Choose a reason for hiding this comment

armano2 commented Aug 6, 2017

michalsnik left a comment •

edited

Loading

armano2 commented Aug 4, 2017 •

edited

Loading

michalsnik commented Aug 4, 2017 •

edited

Loading

armano2 commented Aug 4, 2017 •

edited

Loading

armano2 commented Aug 5, 2017 •

edited

Loading

armano2 Aug 5, 2017 •

edited

Loading

armano2 Aug 5, 2017 •

edited

Loading