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

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

Merged
merged 4 commits into from
Apr 8, 2024
Merged

Conversation

dcousens
Copy link
Member

@dcousens dcousens commented Apr 8, 2024

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!

@dcousens dcousens changed the title Remove deprecated @keystone-6/core/system exports Remove deprecated @keystone-6/core/system package Apr 8, 2024
@dcousens dcousens changed the title Remove deprecated @keystone-6/core/system package Remove deprecated @keystone-6/core/system from package Apr 8, 2024
@dcousens dcousens merged commit 70ffa67 into main Apr 8, 2024
43 checks passed
@dcousens dcousens deleted the major4 branch April 8, 2024 10:40
@dcousens dcousens mentioned this pull request Apr 15, 2024
@gautamsi
Copy link
Member

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

dcousens commented Jun 29, 2024

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

dcousens commented Jun 29, 2024

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

gautamsi commented Jun 29, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants