keystonejs / keystone

The most powerful headless CMS for Node.js — built with GraphQL and React
https://keystonejs.com
MIT License
8.98k stars 1.13k forks source link

Remove deprecated `@keystone-6/core/system` from package #9085

Closed dcousens closed 2 months ago

dcousens commented 2 months ago

This pull request drops @keystone-6/core/system from @keystone-6/core's exports.

If you are using createSystem or createExpressServer, please respond to this pull request and we can talk about alternative options (tl;dr, use getContext).

This change allows us to internally use a different type for KeystoneConfig, which has historically had a number of default behaviours which are difficult to document and maintain.

Additionally, this pull request removes a number of named types, which are generally less helpful by comparison to reaching into the KeystoneConfig type directly. If you wanted these types, please let us know with feedback on this pull request!

gautamsi commented 1 week ago

I see a problem about the internal config. instead of internal config we should have used relaxed KeystoneConfig input the config() function.

Also I see the config function being just returning he config, it makes difficult to create some plugin which wraps the config,

I believe the defaults should be applied in the config before returning. this makes withAuth also receive the default values like the basePath

i will create a PR in hope to get this fixed

dcousens commented 1 week ago

@gautamsi I think that is a good approach, and something I had considered. The only problem is that it is different to the pattern of usage that we have currently, and is thus a breaking change.

dcousens commented 1 week ago

The primary work in this pull request was about removing "defaults" and implicit behaviors from the underlying code, and surface it nearer to the origin.

Moving the defaults to config is now as simple as moving the internals of resolveDefaults, something that wasn't straight forward previously.

gautamsi commented 1 week ago

it is now relatively simple yet still complex due to split config, IMO we should have single config and not split config.

Would you be in favor of plugins config option like this,

export default config({
  // ... other config
  plugins: [withAuth, withTracking] // withTracking is my contrib list plugins
}

plugins would then be run serially by inputting previous config similar to wrapping withTracking(withAuth(config({...}))). Breaking changes would be simple to fix or even not needed as the original wrapping would still be working after I return default config

can create the PR for this as well, this will be a breaking change but we can put this together with the App Router branch to minimize disruption.

Otherwise I am going to create a PR which will just return the resolved config which will be ResolvedKeystoneConfig type (removed __ prefix), plugin authors can use this type instead of KeystonConfig type.

edit created #9189 which should be frictionless :)