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

Updated typings #213

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Updated typings #213

wants to merge 2 commits into from

Conversation

zofiag
Copy link

@zofiag zofiag commented Jul 25, 2019

Summary

Add missing sourceContentType and targetContentType in interface of transformEntriesToType params + enable undefined as a return value in deriveEntryForLocale.

Description

contentType: string; was replaced with

sourceContentType: string;
targetContentType: string;

undefined was added as accepted return value in deriveEntryForLocale.

deriveEntryForLocale: (
    inputFields: ContentFields,
    locale: string
  ) => { [field: string]: any } | undefined;

Motivation and Context

Make current functionalities consistent with the typings.

index.d.ts Outdated Show resolved Hide resolved
@chrislambe
Copy link

chrislambe commented Sep 17, 2019

Not that I necessarily disagree with your styling changes, but it seems like a pretty sure way to get this PR rejected/ignored. I would love to see this change incorporated since the current interface is wildly incorrect compared to the docs. Maybe resubmit with only the meaningful changes?

Edit
It looks like the interface we want is defined right here:
https://github.com/contentful/contentful-migration/blob/f5b3595a862f03510b57d681aad7a53c96f3b33b/src/lib/interfaces/entry-transform-to-type.ts

Any reason we can't just use that instead of duplicating it?

@zofiag
Copy link
Author

zofiag commented Sep 29, 2019

sorry for the automated formatting, is now gone. The linked interface TransformEntryToType could work if we also enable string as a return type from identityKey, so
identityKey: (fromFields: any) => Promise<string> | string. Then we could import that and reuse in index.d.ts for consistency reasons

@pappkamerad
Copy link

what is the status with that? we would also need correct typings.

bgschiller added a commit to bgschiller/contentful-migration that referenced this pull request May 17, 2022
I'm working on updates to deluan/contentful-migrate right now, and am
finding that I need to recreate a lot of type information that already
exists in this library. In addition to being a lot of work, duplicating
that information makes my work fragile. It can easily become out of sync
when the types inside contentful-migration change.

If you add this flag, the type declarations (.d.ts files) will become
part of the npm package, letting me depend on them without rewriting
them by hand.

In contentful#213, @chrislambe comments:
> It looks like the interface we want is defined [in the source]. Any
> reason we can't just use that instead of duplicating it?

The answer to his question is that the declarations are not made
available in the npm package. This PR will fix his problem as well as
mine.
marcolink pushed a commit that referenced this pull request Sep 9, 2022
I'm working on updates to deluan/contentful-migrate right now, and am
finding that I need to recreate a lot of type information that already
exists in this library. In addition to being a lot of work, duplicating
that information makes my work fragile. It can easily become out of sync
when the types inside contentful-migration change.

If you add this flag, the type declarations (.d.ts files) will become
part of the npm package, letting me depend on them without rewriting
them by hand.

In #213, @chrislambe comments:
> It looks like the interface we want is defined [in the source]. Any
> reason we can't just use that instead of duplicating it?

The answer to his question is that the declarations are not made
available in the npm package. This PR will fix his problem as well as
mine.
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.

4 participants