Skip to content

Refactor: clean up what many devs left untouched (improves DX) #2588

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

Open
wants to merge 17 commits into
base: refactor/docs
Choose a base branch
from

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Mar 25, 2025

While working on the docs, I ran into a bunch of structural annoyances I couldn’t ignore anymore. Quick list:

  • Got rid of unnecessary index.ts files, they made imports messier than they needed to be. We’ve got modern IDEs, direct imports are fine.
  • Renamed a few misleadingly named files.
  • Cleaned up types: some were meant for export-only but were being reused internally.
  • Exported finally our actual plugin config options type (ClientOptions), the old deprected one was confusing and outdated, so I removed ConfigParams.
  • Main file imports and exports were a mess, I separated type imports from regular ones and sorted everything alphabetically.
  • the readme badly needed a touch

Great side-effect, the bundles shrink with these changes:

Original size in current master:

dist/contentful-management.browser.js
Size limit: 161 kB
Size:       114.03 kB brotlied

dist/contentful-management.browser.min.js
Size limit: 70 kB
Size:       55.48 kB brotlied

dist/contentful-management.node.js
Size limit: 177 kB
Size:       148.42 kB brotlied

dist/contentful-management.node.min.js
Size limit: 88 kB
Size:       79.58 kB brotlied

I removed the index file in lib/entities and forced the files to use "one layer less" to import types. Also helps with DX.

dist/contentful-management.browser.js
Size limit: 161 kB
Size:       113.06 kB brotlied

dist/contentful-management.browser.min.js
Size limit: 70 kB
Size:       53.24 kB brotlied

dist/contentful-management.node.js
Size limit: 177 kB
Size:       148.09 kB brotlied

dist/contentful-management.node.min.js
Size limit: 88 kB
Size:       77.59 kB brotlied

Then I made sure our exported-types are actually only used to export types

dist/contentful-management.browser.js
Size limit: 161 kB
Size:       112.55 kB brotlied

dist/contentful-management.browser.min.js
Size limit: 70 kB
Size:       53.01 kB brotlied

dist/contentful-management.node.js
Size limit: 177 kB
Size:       147.65 kB brotlied

dist/contentful-management.node.min.js
Size limit: 88 kB
Size:       77.28 kB brotlied

We still have plenty of circular dependencies, lets see if more optimization reduces bundle-size further :)

@axe312ger axe312ger marked this pull request as ready for review March 25, 2025 14:41
@axe312ger axe312ger requested a review from a team as a code owner March 25, 2025 14:41
Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

Just a few small questions. Love the decreased bundle sizes!

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes in the README look great, thanks for addressing this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cheers :)

/**
* @deprecated
*/
export type ClientParams = RestAdapterParams & UserAgentParams
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is already labeled as deprecated, do you think this is safe to remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, as these are used by the legacy client.

The whole legacy vs plain client situation should be adressed by us soon IMHO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@whitelisab so i checked, this is a left over from ages ago and just confuses people. so lets get rid of it! The real config types for plain and legacy client are available for our users

@axe312ger axe312ger requested a review from a team as a code owner April 24, 2025 15:14
axe312ger and others added 11 commits April 24, 2025 17:20
…ensure naming of variable matches naming of type
…ighlight our examples on contentful.com, config options as table, some more cleanups
* docs: categorize navigation for better plain and legacy client destinction

* refactor: move createClient in its own file and rename defaultProps to plainClientDefaultProps

* docs: add descriptions to our entity status check functions

* Update lib/create-client.ts

Co-authored-by: Lisa White <[email protected]>

* Update lib/plain/checks.ts

Co-authored-by: Lisa White <[email protected]>

* refactor: simplify or remove @ts-expect-errors

* chore: set createAssetApi to private again

---------

Co-authored-by: Lisa White <[email protected]>
@axe312ger axe312ger force-pushed the refactor/restructure-to-simplify-and-reduce-circular-dependencies branch from 1e3d873 to 5363aca Compare April 24, 2025 15:20
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.

2 participants