Skip to content

Conversation

andipaetzold
Copy link
Contributor

@andipaetzold andipaetzold commented Mar 26, 2025

SysLink is a frequently used type within CMA.js. Unfortunately, sys.type and sys.linkType are typed as string instead of "Link" and the expected entity name.

In this PR, SysLink is replaced with Link<T>. MetaLinkProps is adjusted accordingly. I also added a generic to MetaSysProps and BasicMetaSysProps to clearly define whether createdBy etc is only a user or could also be an app.

With Link<T> we gain additional type safety as sys.type is typed as "Link" and sys.linkType is typed as T.

SysLink and MetaLinkProps are deprecated and it's recommended to always use Link<T>.

I am not 100% confident on all link types. After an initial reviewed, I'd share this PR internally in case a team spots an inconsistency.

Open questions:

  • Should we make all links to a subject (e.g. sys.createdBy) Link<'User'> | Link<'AppDefinition'>? Or check one by one whether that entity can be accessed by an app?
  • Should we write Link<'User'> | Link<'AppDefinition'> or Link<'User' | 'AppDefinition'>? The result of the type checking should be identical.

This will be released as a breaking change in CMA.js v12

@andipaetzold andipaetzold self-assigned this Mar 26, 2025
@andipaetzold andipaetzold requested a review from marcolink March 26, 2025 14:25
@andipaetzold andipaetzold marked this pull request as ready for review March 26, 2025 14:25
@andipaetzold andipaetzold requested a review from a team as a code owner March 26, 2025 14:25
@marcolink
Copy link
Member

@andipaetzold my thoughts to your questions:
In general, I'd prefer to narrow down the possible actor (User, AppDefinition) as it will make it easier to resolve it if required.
Does entities exposed in the App SDK map to entities that can have AppDefinition as a actor?

About how to write the type - I don't have a preference :)

id: string
}
/**
* @deprecated Use more specific `Link<T>` instead
Copy link
Member

Choose a reason for hiding this comment

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

❤️ for proper deprecation

@andipaetzold andipaetzold changed the title fix: replace SysLink & MetaLinkProps with Link<T> to improve type safety fix: replace SysLink & MetaLinkProps with Link<T> to improve type safety [CFISO-2344] Mar 28, 2025
@andipaetzold
Copy link
Contributor Author

Does entities exposed in the App SDK map to entities that can have AppDefinition as a actor?

Good idea. I went through the list and updated the PR to match the App SDK capabilities

@jjolton-contentful
Copy link
Contributor

Should we write Link<'User'> | Link<'AppDefinition'> or Link<'User' | 'AppDefinition'>? The result of the type checking should be identical.

I did notice a subtle difference between these two options:

Link<'User'> | Link<'AppDefinition'>: preserves the identity of each linkType

Link<'User' | 'AppDefinition'>: allows linkType to be a union, but we lose the ability to discriminate by type in TypeScript

I prefer Link<'User'> | Link<'AppDefinition'>, because then we can do:

if (link.sys.linkType === 'User') {
  // Now TypeScript knows it's a Link<'User'>
}

@andipaetzold andipaetzold requested a review from a team as a code owner April 10, 2025 07:43
@andipaetzold
Copy link
Contributor Author

As we agreed to release this in a new major version, I made a change to completely remove SysLink and MetaLinkProps.

@whitelisab whitelisab changed the base branch from master to release/v12 April 16, 2025 14:49
@whitelisab whitelisab merged commit 3be3fb4 into release/v12 Apr 16, 2025
6 checks passed
@whitelisab whitelisab deleted the remove-syslink-and-metalinkprops branch April 16, 2025 14:49
whitelisab added a commit that referenced this pull request Apr 16, 2025
* BREAKING CHANGE: full ESM support (#2472)

* fix: clean dependencies, delete unused file, migrated overlooked tests from last merge

* refactor: build process now supports TS, ESM, CJS and browser

* build: upgrade to ESM version of contentful-sdk-core

* test: reduce bundle size limits thanks to our optimizations

* build: clean up depdency changes

* chore: typos

* feat: this SDK now also includes its version number in its bundles

* test: fix output integration tests

* build: npm dedupe

* build: latest rollup dependencies

* fix: use with keyword instead of assert to mark import as json

* build: update depndencies related to this branch

* build: make npm happy to resolve our dependencies without --force

* test: ensure new failing typescript lint rules throw warnings only - still should be resolved at some point

* test: fix import paths

* build: ensure json-patch is available for typescript in projects using this api client

* ci: update node version

* fix: adjust import path

* style: ensure prettier doesnt fail on our rollup config file

* style: format code to make prettier lint happy

* style: format code to make prettier lint happy #2

* ci: update circleci config to match our new setup

* ci: cleanup

* test: slightly increase bundle size limits

* feat: introduce MIGRATION.md file

* docs: readme cleanup

* fix: move types definition to top of exports as demanded by node docs

* fix: disable linting on inject-env file

* docs: fix wrong link

---------

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

* fix: replace `SysLink` & `MetaLinkProps` with `Link<T>` to improve type safety [CFISO-2344] (#2590)

* fix: replace `SysLink` & `MetaLinkProps` with `Link<T>` to improve type safety

* chore: prettier

* fix: more specific types for subject links

* fix: snapshots are created by users

* fix: webhook health is created by webhook defs

* fix: webhook call is created by webhook defs

* chore: formatting

* fix: update AiAction links

* fix: `createdBy` etc should be a union, not an object

* fix: remove unused types

* fix: remove import

* feat: narrow `sys.type` to a string literal type for better type safety (#2611)

* feat: narrow `sys.type` to a string literal type for better type safety

* fix: types for `Usage`

* chore: prettier

* feat: align taxonomy update methods with standard [NONE] (#2478)

* fix: make version param required for entry patch method of plain client [DX-34] (#2610)

---------

Co-authored-by: Benedikt Rötsch <[email protected]>
Co-authored-by: Andi Pätzold <[email protected]>
Co-authored-by: LiamStokingerContentful <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants