[Bug] Objects exported with '* as' export aren't merged recursively

[AndrewBogdanovTSS]

Environment

Operating System: Linux Node Version: v18.18.0 Nuxt Version: 3.8.2 CLI Version: 3.10.0 Nitro Version: 2.8.1 Package Manager: npm@9.4.2 Builder: - User Config: devtools, extends Runtime Modules: - Build Modules: -

Reproduction

https://stackblitz.com/edit/github-bcxfrv?file=README.md

Describe the bug

merge app.config from different layers using namespace imports to combine different parts of the config, e.g:

import * as pages from './config/pages'

export default defineAppConfig({
  pages,
})

Check the console:

Expected:

{ nuxt: { buildId: 'dev' }, pages: { foo: { nested: 1 }, bar: 2 } }

Actual:

{ nuxt: { buildId: 'dev' }, pages: { bar: 2 } }

Additional context

This is a regression that was introduced with 6.1.3 release, specifically with this PR https://github.com/unjs/defu/pull/111 @pi0 I'm quite dissapointed with the overall workflow this change was merged with. Test coverage was very small for such fundamental change, but even those tests that were written were skipped, there was no code review. Was it some kind of pre-holiday accident? 🥲 IMO, any changes like this deserves at least minor (not a patch) and better - major version bump because it's a fundamental change to how objects are merged and this is a main goal of this lib.

Logs

No response

[manniL]

@AndrewBogdanovTSS any chance you could provide a defu-only repro?

[AndrewBogdanovTSS]

@manniL I can do it a bit later, but I think you can also just remove skip from unit tests that check this behavior and see that they are failing. We use defu in nuxt free environment and it fails there as well, that's how I found it out initially. Also, reverting to 6.1.2 fixes an issue, so it's 100% related to that newly added code

[manniL]

I can do it a bit later, but I think you can also just remove skip from unit tests that check this behavior and see that they are failing.

@AndrewBogdanovTSS FYI: only the Date test is failing then.

[AndrewBogdanovTSS]

It's because a test for merging objects imported with default as * were never written 😂

[manniL]

That's true. I just did and it fails 🙈

[pi0]

Thanks for reporting this issue dear @AndrewBogdanovTSS and thanks for all help @manniL to investigate and fix this regression. The fix should be available shortly in the next release.

---

This is a regression that was introduced with 6.1.3 release, specifically with this PR https://github.com/unjs/defu/pull/111 @pi0 I'm quite disappointed with the overall workflow this change was merged. Test coverage was very small for such fundamental change, but even those tests that were written were skipped, there was no code review. Was it some kind of pre-holiday accident? 🥲 IMO, any changes like this deserve at least a minor (not a patch) and better - major version bump because it's a fundamental change to how objects are merged and this is a main goal of this lib.

Although it is something unnecessary to discuss here, answering only to not keep as unanswered since honestly hearing it is super frustrating for me while I am taking my Friday evening to stay home and spend time on it!

The fact that you are knowingly saying it was a regression and then asking to release a fix as a breaking change is strange. The main issue was Date objects being wrongly merged and Dates are a really common usage compared to Module import merging.

The initial fix was made by me after doing enough tests AND trusting the code base of is-plain-obj with 22M weekly downloads. Of course, you can call any of us stupid or careless but the fact is-plain-plain-ob was tested and used enough to be trusted, and this issue was never reported to any of the libs should be enough I guess. Your report got fixed in less than 9 hours even though you didn't provide a minimal reproduction.

Wish you the best!

[AndrewBogdanovTSS]

@pi0 first of all I want to apologize if the way I logged an issue hurt your feelings in any way, that wasn't my intention. I would like to clarify couple of things here

The fact that you are knowingly saying it was a regression and then asking to release a fix as a breaking change is strange.

The only thing I was saying is that things that change main functionality should be released as a significant change that is not easely inherited by a dependency change during lock file update, and again, this is just my opinion, a suggestion that is not a form of blame or anything like it.

Your report got fixed in less than 9 hours even though you didn't provide a minimal reproduction.

Initially this issue was reported more than 3 weeks ago in the Nuxt repo (since it was discovered in the Nuxt based project), with a minimal reproduction which is also attached to this bug report. Although I sencerely appreciate your swift reaction in defu repo, I wasn't demanding immidiate attention to it, wasn't locking you in a room with a demand to fix it within a specific timeframe.

Of course, you can call any of us stupid or careless

I didn't call anyone stupid, I only expressed a personal feeling. I think things you build are one of the best among open source projects I've seen for years and of course I have a high expectations from the codebase, but by no means I wanted to stress you out with it, sorry if it felt that way.