nuxt / framework

Old repo of Nuxt 3 framework, now on nuxt/nuxt
https://nuxt.com
10.64k stars 1.05k forks source link

fix(nuxt): support deep assign on empty object for app config #10087

Closed atinux closed 1 year ago

atinux commented 1 year ago

🔗 Linked issue

No linked issue, but a reproduction: https://stackblitz.com/edit/github-mgmeto?file=app.config.ts,app.vue

Tested locally in playground with my fix and works.

❓ Type of change

codesandbox[bot] commented 1 year ago

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Tahul commented 1 year ago

I have a small suggestion about updateAppConfig and the current update strategy.

We currently use deepAssign and deepDelete in sequence so when calling updateAppConfig(content), the content becomes the new appConfig, without mutating the original object reference to preserve reactivity.

The issue with that is that deepDelete will inevitably delete all the missing keys from content, avoiding the possibility to make partial update of the app.config by sending only parts of it.

While this works very good, I think it would be amazing being able to send only partial updates to the app.config, without implying deepDelete.

Also, I don't think deleting keys from the app.config is safe or needed, as if the key is defined it's inevitable that features in the project somehow depend on these keys, and then removing them could lead to runtime throws.

pi0 commented 1 year ago

@Tahul Can you please open an issue with steps to reproduce? Indeed update shouldn't delete missing sub-keys too. BTW we initially did it because it was making strange behavior when a user removes an optional-option from main app.config and HMR was not reflecting that.

atinux commented 1 year ago

The deepDelete is only used in development actually for HMR: https://github.com/nuxt/framework/blob/fix/update-app-config/packages/nuxt/src/app/config.ts#L61

What are the steps that brings a bad DX?

Tahul commented 1 year ago

The deepDelete is only used in development actually for HMR: fix/update-app-config/packages/nuxt/src/app/config.ts#L61

What are the steps that brings a bad DX?

Wow, my bad, sorry for not double-checking that.

I thought the same sequence occurred in HMR and in runtime call.

I was mentioning this lately while working on studio because we allow partial updates for tokens.config and I thought that same partial updates would break with app.config. We are going to switch between holding the complete version of the app.config file in json files to holding only the keys that has been modified from studio for performances purpose.

You can consider this closed, sorry 😓