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

Fix iterateProperties to support arrow functions #1064

Merged
merged 4 commits into from Mar 14, 2020

Conversation

maddocnc
Copy link
Contributor

@maddocnc maddocnc commented Mar 5, 2020

current iterateProperties doesn't support arrow function properties. However it supports functions and iterate their returned objects.

So we can't treat properties from arrow functions same way:

default way works:

data: function () {
  return {
   property: 'value'
  }
},

But following examples won't work:

data: () => {
  return {
   property: 'value'
  }
},

or like this:

data: () => ({
  property: 'value'
}),

Pull request adds support for ArrowFunctionExpression in iterateProperties.

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

Could you add test cases to the following rules.

  • no-dupe-keys
  • no-reserved-keys
  • no-template-shadow

@maddocnc
Copy link
Contributor Author

maddocnc commented Mar 6, 2020

@ota-meshi , added same cases but with 2 types of arrow functions:
() => {return...} and ()=>({})
where was tests with function () {}

@maddocnc maddocnc requested a review from ota-meshi March 6, 2020 13:59
Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@maddocnc
Copy link
Contributor Author

maddocnc commented Mar 8, 2020

Thank you, for approving.

So what's next? We need some other review?
When it could be in npm approximately?

We are going to release project soon where we need this changes, we could use fork there, but this changes seems quite useful for everyone, that's why we made PR.

@ota-meshi
Copy link
Member

Hi @frenchrabbit.
I don't know when the release will be, but I will probably release this change with a beta release (7.x-beta) that supports Vue.js 3.
I would normally release this change in a minor release, but I don't have that time.

@maddocnc
Copy link
Contributor Author

Ok, thank you. Looking forward to see it in release.

@ota-meshi ota-meshi merged commit 7a790bc into vuejs:master Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants