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

chore: Keep Nuxt's 'asyncData' and 'fetch' with 'data' #823

Merged
merged 4 commits into from Aug 17, 2019
Merged

chore: Keep Nuxt's 'asyncData' and 'fetch' with 'data' #823

merged 4 commits into from Aug 17, 2019

Conversation

rchl
Copy link
Contributor

@rchl rchl commented Feb 18, 2019

Change the order of asyncData and fetch properties to be next
to data property. All those are function-wise more or less
equivalent so IMO it makes sense to keep them together. asyncData
and fetch are primarily for setting up component's data.

@manniL

Change the order of asyncData and fetch properties to be next
to data property. All those are function-wise more or less
equivalent so IMO it makes sense to keep them together. asyncData
and fetch are primarily for setting up component's data.
@manniL
Copy link
Contributor

manniL commented Feb 18, 2019

I'd love to get feedback about this from @pi0 and @Clark ☺️

@@ -19,11 +19,9 @@ const defaultOrder = [
'inheritAttrs',
'model',
['props', 'propsData'],
'data',
['asyncData', 'fetch', 'data'],
Copy link

Choose a reason for hiding this comment

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

I suggest moving fetch to the top of asyncData because it is more context less

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi0 but as I understand, having asyncData and fetch in same array means the order can be freely chosen between those. Then it shouldn't be need for moving them around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I have actually updated to have them in explicit order.

@rchl
Copy link
Contributor Author

rchl commented Feb 25, 2019

Ping

@manniL
Copy link
Contributor

manniL commented Feb 25, 2019

@rchl I forgot: could we add head somewhere too?

@rchl
Copy link
Contributor Author

rchl commented Feb 25, 2019

Added after props and before data related props. Scream if you disagree.

@manniL
Copy link
Contributor

manniL commented Feb 25, 2019

@rchl For me it's ok. Anyway, would it make sense at the end, as you can use methods & stuff in there too?

@rchl
Copy link
Contributor Author

rchl commented Feb 25, 2019

True. How about:

  'methods',
  'head',
  ['template', 'render'],

At the very end would work for me too but this possibly makes a bit more sense.

@manniL
Copy link
Contributor

manniL commented Feb 25, 2019

@rchl Agree with the proposed positioning ☺️

@rchl
Copy link
Contributor Author

rchl commented Mar 20, 2019

Seems ready to merge?

@rchl
Copy link
Contributor Author

rchl commented Apr 10, 2019

Merge please. :)

@manniL
Copy link
Contributor

manniL commented Apr 10, 2019

cc @michalsnik @mysticatea 😌

@ota-meshi ota-meshi merged commit 27fc35c into vuejs:master Aug 17, 2019
@vue-bot
Copy link

vue-bot commented Aug 17, 2019

Hey @rchl, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

@vinayakkulkarni
Copy link

OK, this messed up my code. lol

/Users/vinayak/Development/Work/frontend/pages/signup.vue
  101:3  warning  The "methods" property should be above the "head" property on line 90  vue/order-in-components

✖ 145 problems (0 errors, 145 warnings)

@rchl
Copy link
Contributor Author

rchl commented Nov 7, 2019

@vinayakkulkarni it's unfortunate that this fix was lingering for so long. You have an option to either override the rule in your eslint configuration or fix automatically with eslint --fix

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

7 participants