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

Build: cleanup dependencies #2336

Merged
merged 10 commits into from
Jul 17, 2024
Merged

Conversation

axe312ger
Copy link
Collaborator

Some dependencies are obsolete, so lets get rid of them.

Part of #2334

@axe312ger axe312ger marked this pull request as ready for review June 26, 2024 12:27
package.json Outdated
"axios": "^1.7.2",
"contentful-sdk-core": "^8.1.0",
"fast-copy": "^3.0.0",
"lodash.isplainobject": "^4.0.6",
"json-patch": "^0.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

Are we using any implementation from it, or just types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think just the types: Would you move it to dev dependency?

Copy link
Member

Choose a reason for hiding this comment

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

What about not using it at all and maybe changing the imports🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well we support json-patch requests in the API, so we should also provide the correct types. We can fix the imports to use import type to make sure we never include these imports outside of TS

Copy link
Member

Choose a reason for hiding this comment

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

But all current references are only pointing to the types package. Is the plan to already pull it in because we will use the actual implementation in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added it because it was a missing dependency which got installed through sth else.

I guess the original author added it so we don't need to maintain our own type for this action. As we need https://json.schemastore.org/json-patch as a TS type :)

I see two options:

  • use import type and move it to dev dependencies
  • add our own TS type for the json-patch request. Maybe the same as the CTF backend uses? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

what ever it takes to not maintain our own implementation :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcolink #2339

is this worth the changes? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I'd say yes :) - and since you already did it ...

* build: upgrade typescript to v5

* style: update eslint to support latest typescript and apply it (#2338)
@axe312ger axe312ger requested a review from a team as a code owner June 27, 2024 09:42
@axe312ger axe312ger merged commit bfebcfe into test/fix-integration Jul 17, 2024
2 checks passed
@axe312ger axe312ger deleted the build/cleanup-dependencies branch July 17, 2024 14:15
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