fix(utils): getRootNodes flattens only one depth of children

[cexbrayat]

TS v4.7 is now trying to compute the type more accurately in flat() and Infinity is not playing very well: it throws

src/utils/getRootNodes.ts:28:22 - error TS2589: Type instantiation is excessively deep and possibly infinite

I'm wondering if we really need to flatten infinitely, as the following lines call getRootNodes recursively on each element. But as I'm a bit fuzzy on how this part of the code works, I'd like more informed opinions 😉

[netlify]

✅ Deploy Preview for vue-test-utils-docs ready!

Name	Link
🔨 Latest commit	6373f80
🔍 Latest deploy log	https://app.netlify.com/sites/vue-test-utils-docs/deploys/6290902d29bd87000db64ae6
😎 Deploy Preview	https://deploy-preview-1546--vue-test-utils-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[lmiller1990]

I also have no idea how this works. The default depth is 1, which is obviously different from Infinity. That said, if the tests pass, I guess it's fine? I'd rather not have code we don't understand - if the tests pass, I'm guessing the default of 1 is fine.

Another alternative would be Number.MAX_SAFE_INTEGER , not sure if we would have the same problem as Infinity. What do you think about just merging this as-is? I'm happy to do so - if it breaks something, someone will report an issue and we can add a test capturing why Infinity is necessary.

[cexbrayat]

I think TS blows when the depth is > 20. Maybe @freakzlike or @xanf can tell us if depth 1 is fine.

If not, I guess we can merge and release, but maybe as a beta version to avoid breaking too many people if that doesn't work? That way we can test on our projects and release a stable version when we are OK with the changes.

[lmiller1990]

I'm not sure the value of a beta branch - who's going to test it other than us? Maybe we can run this fix against our code bases and see how it goes.

Unless someone can come up with a failing case, I think we need to have confidence in our test suite.

[freakzlike]

Let me check something in the upcoming day. I might have a case in my mind where this happens

[cexbrayat]

@lmiller1990 Some teams are running Renovate/dependabot updates with unstable versions to check for breaking changes (I know my teams do). A beta release makes it easy to test and makes sure we're not breaking everybody who updates automatically to the latest stable version 😉 If that's too much hassle for you to cut the releases, I can help if you grant me the permissions on NPM

@freakzlike Thanks, that would be great!

[lmiller1990]

@cexbrayat curious what you mean by beta here - like 2.0.0-beta.1? Feels kind of weird to have a beta for what is effectively a patch release. Isn't the whole ideal of dependabot is it'll update to the latest, and your tests will fail if there's a problem (then you can file an issue).

How will anyone other than people reading this thread (which is just us, really) know to try the beta?

As for permissions, you will need to ping Evan - it sure would be good for someone other than me to be able to cut releases 👍

[cexbrayat]

Yeah it could be 2.0.1-beta.0 or 2.1.0-beta.0. I prefer to be on the safer side, and not cut a latest release with changes that might be breaking: we know a lot of people install the latest version blindly, and then they get a bad opinion of the library because there was a breaking regression in a patch release.

A beta release does not cost much, and people following the repo can see the beta release and give it a try. I agree this won't be a ton of people, but it's still better than going blindly IMHO. And even for people using a bot, having a failing bot PR adds work that is unnecessary: they have to investigate why, check for opened issues, open a new one, etc. I think this is OK when opting for unstable releases but not for stable ones. Again, this is just my POV. No worries if you prefer to go straight to production 😉

[lmiller1990]

Ok, I will do a beta release. I will do it tomorrow.

[freakzlike]

I was not able to reproduce the usage of .flat() and is not used within one of our tests. The type definition says it is possible, but I don't know how

export declare type VNodeArrayChildren = Array<VNodeArrayChildren | VNodeChildAtom>

I tried this, but the arrays get wrapped by a vnode fragment and added by the recursive call:

const wrapper = mount({
  render() {
    return [
      h('span', 'Text 1'),
      [
        h('span', 'Text 2'),
        h('span', 'Text 3'),
        [h('span', 'Text 4'), h('span', 'Text 5')]
      ]
    ]
  }
})

Might be that the .flat() is actually not required, but I'm not sure. I pushed the test cases I have created to #1569. Feel free to merge or close