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: don't allow additional parameters #2491

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Conversation

andipaetzold
Copy link
Contributor

@andipaetzold andipaetzold commented Dec 2, 2024

Summary

Have stricter types and forbid passing additional params

Description

Currently, the following code is valid:

import { createClient } from "contentful-management";

const client = createClient({ accessToken: "..." }, { type: "plain" });
await client.entry.getMany({
  foo: "bar",
});

We allow passing additional params and loose type-safety that way. The goal with this PR is to improve type-safety and DX by having stricter types and forbidding passing additional params.

With this change, the developer will receive an error:

Object literal may only specify known properties, and 'foo' does not exist in type 'OptionalDefaults<GetSpaceEnvironmentParams & QueryParams>'

As we are touching types, you can consider this a breaking change. However, argued that way, every change is breaking.

To test this change, I introduced vitest typechecking tests.

To make typescript happy, skipLibCheck had to be enabled. But, IMO, that's anyway recommended to be enabled. This should speed-up general type checking as well.

Checklist (check all before merging)

  • Both unit and integration tests are passing
  • There are no breaking changes
  • Changes are reflected in the documentation

@andipaetzold andipaetzold self-assigned this Dec 2, 2024
@andipaetzold andipaetzold marked this pull request as ready for review December 2, 2024 14:57
@andipaetzold andipaetzold requested a review from a team as a code owner December 2, 2024 14:57
@andipaetzold andipaetzold requested a review from mdmjg December 2, 2024 14:59
Copy link
Contributor

@jjolton-contentful jjolton-contentful left a comment

Choose a reason for hiding this comment

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

Thanks very much for this DX enhancement @andipaetzold 🙇

I agree with you in your assessment that although the change tightens type restrictions, it is unlikely to break existing code unless that code relies on passing unintended parameters, so I also see this as more of a bug fix than a breaking change.

Copy link
Contributor

@denkristoffer denkristoffer left a comment

Choose a reason for hiding this comment

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

I absolutely think type changes should be handled like code changes but in this case I think it's fine and can be considered a fix 🙂

@@ -9,7 +9,8 @@
"isolatedModules": true,
"esModuleInterop": true,
"noImplicitThis": false,
"typeRoots": ["node_modules/@types"]
"typeRoots": ["node_modules/@types"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a question but isn't this the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, afaik, it is. This line can probably be removed.

@andipaetzold andipaetzold merged commit 885335c into master Dec 3, 2024
2 checks passed
@andipaetzold andipaetzold deleted the fix-OptionalDefaults branch December 3, 2024 12:36
@contentful-automation
Copy link
Contributor

🎉 This PR is included in version 11.40.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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