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

Move cloudinary, unsplash and oembed out of fields package and oembed-adapters into new fields-oembed package #3280

Merged
merged 39 commits into from
Aug 10, 2020

Conversation

MadeByMike
Copy link
Contributor

@MadeByMike MadeByMike commented Jul 23, 2020

Lots of files moved around here but not functional changes. I'm trying to consolidate the "core field packages" and isolate them from other field types. I intend to make this distinction clearer in docs in a future PR.

Moved @keystonejs/fields-cloudinary-image, @keystonejs/fields-unsplash and @keystonejs/fields-oembed out of the core fields package @keystonejs/fields.

@changeset-bot
Copy link

changeset-bot bot commented Jul 23, 2020

🦋 Changeset is good to go

Latest commit: 836079c

We got this.

This PR includes changesets to release 32 packages
Name Type
@keystonejs/oembed-adapters Major
@keystonejs/fields-oembed Major
@keystonejs/fields Major
@keystonejs/fields-cloudinary-image Major
@keystonejs/fields-unsplash Major
@keystonejs/fields-authed-relationship Patch
@keystonejs/fields-auto-increment Patch
@keystonejs/fields-wysiwyg-tinymce Patch
@keystonejs/fields-mongoid Patch
@keystonejs/cypress-project-basic Patch
@keystonejs/demo-project-blog Patch
@keystonejs/app-admin-ui Patch
@keystonejs/auth-passport Patch
@keystonejs/auth-password Patch
@keystonejs/fields-color Patch
@keystonejs/fields-content Patch
@keystonejs/fields-location-google Patch
@keystonejs/fields-markdown Patch
@keystonejs/keystone Patch
@keystonejs/list-plugins Patch
@keystonejs/demo-custom-fields Patch
@keystonejs/demo-project-meetup Patch
@keystonejs/demo-project-todo Patch
@keystonejs/cypress-project-access-control Patch
@keystonejs/cypress-project-client-validation Patch
@keystonejs/cypress-project-login Patch
@keystonejs/cypress-project-social-login Patch
@keystonejs/api-tests Patch
@keystonejs/benchmarks Patch
@keystonejs/example-projects-nuxt Patch
@keystonejs/example-projects-starter Patch
@keystonejs/example-projects-todo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

test-projects/yarn.lock Outdated Show resolved Hide resolved
@jesstelford
Copy link
Contributor

Even though this is a major, can we export some dummy "fields" with the old names that simply throw new Error('This field has moved to @foo/bar')? That'd make updating / migrating much simpler than unexpected missing exports.

@MadeByMike
Copy link
Contributor Author

Even though this is a major, can we export some dummy "fields" with the old names that simply throw new Error('This field has moved to @foo/bar')? That'd make updating / migrating much simpler than unexpected missing exports.

Yes, 100%. I was hoping to be able to continue to export the fields package with a deprecation warning, but circular references.

Adding dummy fields is a next best option.

@MadeByMike MadeByMike requested a review from timleslie July 27, 2020 00:39
@MadeByMike MadeByMike force-pushed the split-field-packages branch from f1b78f7 to f4dd988 Compare July 27, 2020 00:50
Copy link
Member

@JedWatson JedWatson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a documentation mishap.

Aside from that, I'm strongly in favour of also extracting the Location field since (like these others) it's tightly coupled to an external service provider (Google) -- let's turn that into fields-location-google (following our fields-content-tinymce convention, you could have other location fields backed by other services)

We used to have a Location field in keystone 4 that had support for Google but was also useful independently; the Location field in keystone 5 specifically stores a googlePlaceID field so I don't see it as belonging in the core set.

Might as well include that in this breaking change set.

Also, our Content field package is currently mis-named; should be fields-content not field-content. Given the theme of this PR/release, might be a good time to deprecate the misnamed package and publish the new one?

.changeset/pretty-poems-explode.md Outdated Show resolved Hide resolved
@MadeByMike MadeByMike changed the title Split fields packages Move cloudinary, unsplash and oembed fields packages Jul 28, 2020
@emmatown emmatown changed the title Move cloudinary, unsplash and oembed fields packages Move cloudinary, unsplash and oembed out of fields packages and oembed-adapters into new fields-oembed Aug 9, 2020
@emmatown emmatown changed the title Move cloudinary, unsplash and oembed out of fields packages and oembed-adapters into new fields-oembed Move cloudinary, unsplash and oembed out of fields packages and oembed-adapters into new fields-oembed package Aug 9, 2020
@emmatown emmatown changed the title Move cloudinary, unsplash and oembed out of fields packages and oembed-adapters into new fields-oembed package Move cloudinary, unsplash and oembed out of fields package and oembed-adapters into new fields-oembed package Aug 9, 2020
Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me 👍

@emmatown emmatown merged commit 1d90687 into master Aug 10, 2020
@emmatown emmatown deleted the split-field-packages branch August 10, 2020 00:24
@github-actions github-actions bot mentioned this pull request Aug 10, 2020
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.

5 participants