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 filters export from @keystone-6/core/types #7919

Merged
merged 5 commits into from
Sep 27, 2022
Merged

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Sep 13, 2022

The filters export isn't documented or well designed for public consumption, so we're making it private, custom field types can define filters themselves like https://github.com/keystonejs/keystone/blob/main/examples/custom-field/3-pair-field/index.ts

@vercel
Copy link

vercel bot commented Sep 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
keystone-next-docs ✅ Ready (Inspect) Visit Preview Sep 13, 2022 at 2:53AM (UTC)

@changeset-bot

This comment was marked as resolved.

@vercel vercel bot temporarily deployed to Preview September 13, 2022 01:25 Inactive
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 13, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 13dcbd8:

Sandbox Source
@keystone-6/sandbox Configuration

@vercel vercel bot temporarily deployed to Preview September 13, 2022 01:40 Inactive
@vercel vercel bot temporarily deployed to Preview September 13, 2022 02:01 Inactive
@vercel vercel bot temporarily deployed to Preview September 13, 2022 02:53 Inactive
@emmatown emmatown marked this pull request as ready for review September 13, 2022 03:11
@emmatown emmatown requested a review from a team September 13, 2022 03:12
@dcousens dcousens assigned dcousens and unassigned emmatown Sep 26, 2022
@dcousens dcousens merged commit 51ab286 into main Sep 27, 2022
@dcousens dcousens deleted the remove-filters-export branch September 27, 2022 06:22
@gautamsi
Copy link
Member

gautamsi commented Oct 23, 2022

This should have been documented (minimal if not comprehensive) instead of removed exports.

I have to augment filters for timestamp field, caveat is that the controller can not be overridden by passing "ui.views" param. You have to write full custom field for this purpose. Now I ave to copy whole set of files from keystone just for common filters.

Also the custom Fields should be able to make use of default filter in most cases.

Should there be way to override controller when setting "ui.views" by having "controller" exports?

@dcousens
Copy link
Member

dcousens commented Oct 23, 2022

@gautamsi we are sympathetic to trying to make this experience less tedious, for ourselves and the community.

Before we made this change, we found that while reviewing the ecosystem, and our own examples, that filters as an export was being used without consideration of how the filters might apply to the use-case.
In actuality, the filters often made little sense for how they were being applied, and they should have been written differently.

We know that removing the export is a harsh approach, but we really want to improve the developer experience; and we don't want to encourage developers inadvertently writing buggy code.

If you had any specific examples where writing these filters is particular painful in updating your packages, please report your experience! We can then work together in improving the DX for everyone.

Should there be way to override controller when setting "ui.views" by having "controller" exports?

This is a good question, I'll bring this up with the team next week.

Also the custom Fields should be able to make use of default filter in most cases.

I think the point should be that filters should be easier to write, but not necessarily "defaulted" or "copied".
It is our understanding that in actuality you only want some subset of the default filters from a reference field.

@dcousens
Copy link
Member

dcousens commented Oct 23, 2022

My understanding at the moment is that writing custom fields, is painful, confusing, error prone, and now, verbose.

I want to introduce new functions (in a non-breaking way) that make writing custom fields easier and understandable.
I will highly appreciate any feedback I can receive on future pull requests to that end.
If you're open to that, I will add you as a non-blocking reviewer (or at least ping you) where I can.

@gautamsi
Copy link
Member

thanks @dcousens for the quick answer. I have opened a pull request which may fix this if it was a bug. I am able to use this with patch-package and the changes works great.

hopefully we will have simpler story for extending field with filters. If the PR goes through as patch soon I do not have to worry about the filters. I usually extend existing fields and that does not require writing full set of code for custom field while preserving all the core functionality.

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.

3 participants