Skip to content

chore(clerk-js): Apply deprecation warnings#1779

Merged
dimkl merged 8 commits intomainfrom
apply-deprecation-warnings-clerk-js
Sep 29, 2023
Merged

chore(clerk-js): Apply deprecation warnings#1779
dimkl merged 8 commits intomainfrom
apply-deprecation-warnings-clerk-js

Conversation

@dimkl
Copy link
Copy Markdown
Contributor

@dimkl dimkl commented Sep 25, 2023

Description

Review it per commit

Deprecation warnings added in @clerk/clerk-js:

  • Organization.create() using string parameter
  • Organization.retrieve() limit & offset
  • Clerk.getOrganizationMemberships()
  • svgUrl
  • avatarUrl/logoUrl/faviconUrl/profileImageUrl

Notes: We cannot show deprecation warnings in console for deprecated type properties (eg UserJSON.profile_image_url)

TODO:

  • support deprecation warnings for nested props in separate PR

Relevant PRs:

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

@dimkl dimkl self-assigned this Sep 25, 2023
@dimkl dimkl requested a review from a team as a code owner September 25, 2023 19:10
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 25, 2023

🦋 Changeset detected

Latest commit: af4b7ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-js Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dimkl dimkl force-pushed the apply-deprecation-warnings-clerk-js branch from 18537a5 to 4dde358 Compare September 28, 2023 11:04
@dimkl dimkl changed the title chore(clerk-js): Apply deprecation warnings [WIP] chore(clerk-js): Apply deprecation warnings Sep 28, 2023
@dimkl
Copy link
Copy Markdown
Contributor Author

dimkl commented Sep 28, 2023

@nikosdouvlis PTAL

The logoUrl properties from the snapshots even though they still exist in those classes because of the way the deprecation helper works to mark the property as deprecated.
The deprecation helper redefine the property in the class prototype using a getter/setter that shows a warning in getter using the Object.defineProperty(). By redefining a property, that property is not considered class own property any more which causes the jest snapshot and the JSON.stringify() to omit that property.
This is a side-effect of the deprecation implementation and not a spec but i think that we are okay to proceed with omitting the properties from snapshots.

ref: 4dde358

@dimkl dimkl changed the title chore(clerk-js): Apply deprecation warnings chore(clerk-js): Apply deprecation warnings [WIP] Sep 28, 2023
@panteliselef
Copy link
Copy Markdown
Contributor

@dimkl could we skip deprecating nested properties for this PR and resolve it in a followup ?

dimkl and others added 8 commits September 29, 2023 15:21
…ties

The `logoUrl` properties from the snapshots even though they still
exist in those classes because of the way the deprecation helper
works to mark the property as deprecated.
The deprecation helper redefine the property in the class prototype
using a getter/setter that shows a warning in getter using the
Object.defineProperty(). By redefining a property, that property
is not considered class own property any more which causes the
jest snapshot and the JSON.stringify() to omit that property.
This is a side-effect of the deprecation implementation and not a
spec but i think that we are okay to proceed with omitting the
properties from snapshots.
@dimkl dimkl force-pushed the apply-deprecation-warnings-clerk-js branch from 7ea445f to af4b7ba Compare September 29, 2023 12:21
@dimkl dimkl enabled auto-merge September 29, 2023 12:21
@dimkl dimkl changed the title chore(clerk-js): Apply deprecation warnings [WIP] chore(clerk-js): Apply deprecation warnings Sep 29, 2023
@dimkl dimkl disabled auto-merge September 29, 2023 12:21
@dimkl dimkl added this pull request to the merge queue Sep 29, 2023
Merged via the queue into main with commit f3f6431 Sep 29, 2023
@dimkl dimkl deleted the apply-deprecation-warnings-clerk-js branch September 29, 2023 12:41
@clerk-cookie clerk-cookie mentioned this pull request Sep 29, 2023
@clerk-cookie
Copy link
Copy Markdown
Collaborator

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@clerk clerk locked as resolved and limited conversation to collaborators Sep 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants