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

fix(chore): SYS type declaration #398

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Conversation

marcolink
Copy link
Member

@marcolink marcolink commented Apr 16, 2020

Description

It turned out changes made earlier to SYS.contentType are too strict.
Even though, there are rare cases where contentType can be missing, we will keep it required for now.

This problem has been reported in context of another PR

Update:
We also identified space as an optional field and added the changes to this PR.

It turned out changes made earlier to `SYS.contentType` are too strict.
Even though, there are rare cases where `contentType` can be missing, we will keep it required for now.
@marcolink marcolink requested a review from phoebeschmidt April 16, 2020 14:53
@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #398 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #398   +/-   ##
=======================================
  Coverage   61.90%   61.90%           
=======================================
  Files          11       11           
  Lines        1672     1672           
  Branches      270      270           
=======================================
  Hits         1035     1035           
  Misses        637      637           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08810d1...f3f93e4. Read the comment docs.

@ghost
Copy link

ghost commented Apr 16, 2020

@marcolink space seems to be still a required field, even though in the original PR's description it's said to be left as an optional field to prevent breaking changes.

This is the one change that breaks our build 🙂

stevenpetryk pushed a commit to intercom/contentful-typescript-codegen that referenced this pull request Apr 16, 2020
Contentful changed their typings for `space` in a minor version update, meaning that users of this
library had incorrect types.

BREAKING CHANGE: Drops compatibility with `contentful` versions lower than 7.14.2

Closes #26 but blocked by contentful/contentful.js#398
stevenpetryk pushed a commit to intercom/contentful-typescript-codegen that referenced this pull request Apr 16, 2020
Contentful changed their typings for `space` in a minor version update, meaning that users of this library had incorrect types.

BREAKING CHANGE: Drops compatibility with `contentful` versions lower than 7.14.2

Closes #26 but blocked by contentful/contentful.js#398
Make the newly added field space optional.
@marcolink
Copy link
Member Author

@marcolink space seems to be still a required field, even though in the original PR's description it's said to be left as an optional field to prevent breaking changes.

This is the one change that breaks our build 🙂

@tomimaen
Thanks for your fast and helpful input on this issue.

I changed space to be optional .
Can you confirm this solves the issue you faced in your build process?

@marcolink marcolink requested a review from phoebeschmidt April 17, 2020 07:33
@ghost
Copy link

ghost commented Apr 17, 2020

@marcolink Thank you for your fast fix on this one! I can confirm that the build is successful with this change 👍

@marcolink marcolink merged commit 916d082 into master Apr 17, 2020
@marcolink marcolink deleted the fix/sys-type-declaration branch April 17, 2020 10:52
@phoebeschmidt
Copy link
Contributor

🎉 This PR is included in version 7.14.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Aligertor
Copy link

Im not sure if somebody cares but i do not like the changes to contentType made here.

The "rare cases" can be produced very reliable imho.
The case i have in mind is:

  • Entity A has a linked Entity B
  • your editor accidentally deleted Entry B (for what ever reason)
  • Entity A now remains with a reference to a non existing Entity
  • When querying Entity A now the referenced Entity will have a Sys that has no contentType

This is a case that can happen eventually.
As long as it is possible to delete referenced Entities on https://www.contentful.com/ contentType should be optional.
Imho the developer of the software should be responsible his software handles this error gracefully even so it is a clear mistake on the side of the editor.
Typescript is exactly the tool that should support me in this imho.

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.

3 participants