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

Basic attribute inheritance #4049

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

A5rocks
Copy link

@A5rocks A5rocks commented Mar 8, 2024

This PR adds attribute inheritance irrelevant of the inheritAttrs configuration option (for now). This is in pursuit of making strictTemplates more useful.

I'm not sure how to check inheritAttrs as a follow up: I suppose probably scriptSetupRanges/scriptRanges?


Two things I noticed (not sure if these are already tracked or if I just am incorrect about these):

  • @vue-expect-error doesn't seem to error if the next statement doesn't error
  • components as a direct child of <template> just don't seem to get typed? (this must be expected behavior that I just don't get given that I don't use vue much...)

@A5rocks A5rocks marked this pull request as draft March 8, 2024 06:51
@A5rocks
Copy link
Author

A5rocks commented Mar 8, 2024

Sorry, I should have run all the tests locally, not just the ones with strictTemplate. Some of the snapshot diffs look bad.

@A5rocks
Copy link
Author

A5rocks commented Mar 13, 2024

I don't understand some of the test failures, because it looks like e.g. Child is just not being picked up as a type? Which is strange cause I don't see what I did that could cause that.

... There has to be a better way to approach this. This approach sucks.

@so1ve
Copy link
Member

so1ve commented Mar 14, 2024

Are you still working on this? There are multiple cases to handle.

Scenario 1: Only one root tag

<template>
  <div></div>
</template>

Should inherit div's attributes.

Scenario 2: Multiple root tags

<template>
  <div class="111"></div>
  <div class="111"></div>
</template>

Playground

Should not inherit.

Scenario 3: Using v-bind without inheritAttrs: false

<template>
  <div class="111">
    <span v-bind="$attrs" class="222"></span>
  </div>
</template>

Playground

Should inherit both span and div's attributes.

Scenario 4: Using v-bind with inheritAttrs: false

<script>
export default {
  inheritAttrs: false
}
</script>

<template>
  <div class="111">
    <span v-bind="$attrs" class="222"></span>
  </div>
</template>

Playground

Should inherit span's attributes.

@A5rocks
Copy link
Author

A5rocks commented Mar 14, 2024

Yeah I have scenario 1 and 2, I'm not planning on ever supporting scenario 3, and I'm not sure how to approach scenario 4. (I'm not sure how to reach in and get the value of inheritAttrs... and tracing usage of useAttrs() and $attrs seems annoying.)

But I'm running into roadblocks for the more trivial task of "always inherit attrs from the child, if there's only 1" as seen by failing test cases here... I tried fixing one thing (the latest commit) and it both breaks component typing in some cases and also breaks what initially worked w/r/t inheriting attributes.

Do you think there's a better way to approach implementing this?

@so1ve
Copy link
Member

so1ve commented Mar 15, 2024

I can give it a shot, but I prefer to work in a separate PR to avoid git reset. I can add you as a co-author. WDYT? 👀

@A5rocks
Copy link
Author

A5rocks commented Mar 15, 2024

Sure, that would work! I only started this cause I made some dumb mistakes that would have been caught with strictTemplates... which I can't rely on due to false positives.

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