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(utils): getRootNodes flattens only one depth of children #1546

Merged
merged 1 commit into from Jun 2, 2022

Conversation

cexbrayat
Copy link
Member

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 😉

@cexbrayat cexbrayat requested a review from freakzlike May 27, 2022 08:47
@netlify
Copy link

netlify bot commented May 27, 2022

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...

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Collaborator

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

@cexbrayat
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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

@lmiller1990 lmiller1990 merged commit cbf90ae into vuejs:main Jun 2, 2022
@freakzlike
Copy link
Collaborator

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

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

3 participants