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

WIP: fix #6913, feat($compiler): add support of v-local directive to provide a way to… #7325

Closed
wants to merge 1 commit into from

Conversation

Kingwl
Copy link
Member

@Kingwl Kingwl commented Dec 27, 2017

… set local variable in template

fix #6913

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@Kingwl Kingwl changed the title fix #6913 feat($compiler): add support of v-local directive to provide a way to… fix #6913, feat($compiler): add support of v-local directive to provide a way to… Dec 27, 2017
describe('Directive v-local', () => {
it('should work', () => {
const vm = new Vue({
template: '<div><span v-local="v as foo.bar">{{v.baz}}</span></div>',
Copy link
Member

@posva posva Dec 27, 2017

Choose a reason for hiding this comment

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

I'd rather see the other way around foo.bar as v or a different more js friendly syntax (which allows to create multiple variables):

<div v-local:v="foo.bar" v-lobal:baz="foo.bar.baz">

At the end, it would still be valid js. It can even be aliased to something like <div $v="foo.bar"

Copy link
Member

@posva posva Dec 27, 2017

Choose a reason for hiding this comment

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

Those are my thoughts on the feature. That being said, I feel that this crosses the line of Javascript/Magic even though I see when it can be comfortable to use 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

)
})

it('generate v-local directive with v-for', () => {

Choose a reason for hiding this comment

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

I think we need another test for combining v-for, scoped slot and v-local together as scoped vFor is handled separately IIRC.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated,

  1. add some test for scoped slot
  2. fix invalid v-local in scoped-slot with template tag

Choose a reason for hiding this comment

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

After some deeper thoughts, v-local might be harder than I originally estimated. The root problem is it adds a new way to introduce new variable in template. After this, we will have three way to introduce new variables:

  1. v-for
  2. slot-scope
  3. v-local

With 1 and 2 we already have some ordering issue, #6817. I would say more variable introduction isn't scalable in current implementation. For example, <div v-local:items="foo.deep.prop" v-for="item in items"> is still missing in test. And supporting such usage will require yet another patch in genFor.

@Austio
Copy link

Austio commented Dec 29, 2017

👋 community user here adding thoughts

i appreciate the interest in this feature and also respect your time in creating, however I don't think that increasing the scope, complexity and maintenance of Vue core is worth the cost of this feature because it could be handled in userland.

@Kingwl Kingwl changed the title fix #6913, feat($compiler): add support of v-local directive to provide a way to… WIP: fix #6913, feat($compiler): add support of v-local directive to provide a way to… Dec 29, 2017
@HerringtonDarkholme
Copy link
Member

@Austio It's happy (and rare) to see users ask for not adding a feature 😄 . I do share the same sentiment with you. Let's move the discussion to the original issue #6913 and keep pull request thread for code review.

@yyx990803
Copy link
Member

I'm closing this since I share a similar concern (the tradeoff between codebase complexity vs. the benefits).

Instead, I think we should look at how to make scoped slots syntax even simpler so a less-verbose userland helper component becomes possible.

@pandichef
Copy link

pandichef commented Aug 4, 2020

@yyx990803 @posva

I'm closing this since I share a similar concern (the tradeoff between codebase complexity vs. the benefits).

Instead, I think we should look at how to make scoped slots syntax even simpler so a less-verbose userland helper component becomes possible.

I think this one should be reconsidered. I think people are moving to Alpine.js because it's so easy to sprinkle declarative rendering in server-side templates. At the very minimum, I'd suggest some documentation on how to do this in Vue.js.

As an example, you can make "Django components" like this...

In base.html, you have:

<h1>Welcome to my site</h1>
<div x-data='{hello: "World"}'>  <!-- x-data is Alpine's syntax for declaring state/data variables -->
  <div v-text="hello"></div>
  {% include "MySimpleVueToggleButton.html" with prop1="Hello" prop2="World" only %}
</div>

In MySimpleVueToggleButton.html:

<div v-data='{myvar: "Off"}'>
  {{ prop1 }} <!-- This is a Django mustache, not Vue.js -->
  {{ prop2 }} <!-- This is a Django mustache, not Vue.js -->
  <button type="button" v-on:click="myvar = (myvar === 'Off') ? 'On' : 'Off'">{{ myvar }}</button>
</div>

Note that MySimpleVueToggleButton.html is a nested component using ordinary markup.

I'd love to do this in Vue.js, but it's not clear (at least to me) how to do so with the current docs. Please let me if I'm missing something. Thanks!

This was referenced Dec 1, 2021
@nfplee
Copy link

nfplee commented Dec 1, 2021

@yyx990803 @posva

I agree with @pandichef that this should be reconsidered. This was closed before the release of petite-vue and it would make it easier for people converting from petite-vue to the full Vue experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hope to have a way to set scope of variable
7 participants