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: Always wrap string passed to slot in a helper component #815

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

xanf
Copy link
Collaborator

@xanf xanf commented Aug 1, 2021

I've originally encountered this issue while porting bootstrap-vue to Vue3

All known (at least to me and in mentioned stack overflow 😛 question) methods of checking if something is HTML reports false for <col><col><col> because <col> is considered valid tag only inside table -> colgroup

In order to fix that I simply removed check if it is HTML and always wrap slot provided as string into <SlotWrapper>
This has a downside - if code under test relies on knowing that slot will always have raw strings - it will be confused by wrapper. User could workaround it with passing slot as function:

slots: {
  'some-slot': () => h('Something which should be just plain old string')
}

I consider this way more rare case, and what is more important - people who manipulate slot contents in that way are usually far more expert to understand the root cause of the issue and to discover (or find this PR) workaround.

For original issue - it took me a while to realize why slot work for certain HTML string and does not work for another when the only difference is tag

@xanf xanf force-pushed the xanf-always-wrap-slot branch from 14750d1 to 5e04525 Compare August 1, 2021 00:41
@lmiller1990
Copy link
Member

isHTML was introduced here: #599 I am surprised this did not break one of those tests.

This would technically be a breaking change (at least for existing V2 users). Any chance you could write something I can include in the release notes explain the breaking change and how to work around it? Just a paragraph with a snippet or two would be great.

@lmiller1990 lmiller1990 self-requested a review August 5, 2021 03:28
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems reasonable, just left one comment - request for something to include in release notes explaining the breaking change.

@xanf
Copy link
Collaborator Author

xanf commented Aug 6, 2021

This would technically be a breaking change (at least for existing V2 users). Any chance you could write something I can include in the release notes explain the breaking change and how to work around it? Just a paragraph with a snippet or two would be great.

Well, I'm not sure this is a breaking change. For me it feels more like a fix for perfectly valid use case:

 it('allows passing a scoped slot via string with no HTML inside without template tag', () => {
      const wrapper = mount(ComponentWithSlots, {
        slots: {
          scoped: 'Just a plain {{ params.aBoolean }} {{ params.aString }}'
        }
      })

      expect(wrapper.find('.scoped').text()).toEqual('Just a plain true string')
    })

I've added this test in a recent commit.
WDYT of something like:

Note: if you're passing raw strings to a slot of the component - these are now always wrapped into helper component (previously wrapping occurred only if string contained HTML. If your component relies on raw string being passed to slot without any specific wrappers, pass render function to slot content wrapping your string in h():

 it('allows passing a scoped slot via string with no HTML inside without template tag', () => {
      const wrapper = mount(ComponentWithSlots, {
        slots: {
          // this one will be wrapped with helper component
          scoped: 'Just a plain string',
          // this one will be passed to slot as plain string
          otherScoped: () => h('Just a plain string'),
        }
      })

      expect(wrapper.find('.scoped').text()).toEqual('Just a plain true string')
    })

Feel free to edit this for making most easy-to-understand description

@xanf
Copy link
Collaborator Author

xanf commented Aug 9, 2021

@lmiller1990 Ignore my explanation, if #844 this will effectively hide any "slot wrapper" stuff from end users, making this explanation wrong and unneeded

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

LGTM I think - still a little uneasy, hope we won't break anything (eg if ppl are relying on some specific logic around slots[0].children).

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.

2 participants