Skip to content

chore(idFromString): make error message friendlier#2200

Open
thiskevinwang wants to merge 1 commit into
cloudflare:mainfrom
thiskevinwang:durable-object-id-error-message
Open

chore(idFromString): make error message friendlier#2200
thiskevinwang wants to merge 1 commit into
cloudflare:mainfrom
thiskevinwang:durable-object-id-error-message

Conversation

@thiskevinwang

@thiskevinwang thiskevinwang commented May 31, 2024

Copy link
Copy Markdown

Description

The error alone is a bit cryptic, and if you're seeing it, the solution is somewhat buried in docs: https://developers.cloudflare.com/durable-objects/best-practices/access-durable-objects-from-a-worker/#parse-previously-created-ids-from-strings

This makes the idFromString error slightly friendlier

@thiskevinwang thiskevinwang requested review from a team as code owners May 31, 2024 03:32
@thiskevinwang thiskevinwang requested review from jp4a50 and mikea May 31, 2024 03:32
@github-actions

github-actions Bot commented May 31, 2024

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@thiskevinwang thiskevinwang changed the title chore(idFromtString): make error message friendlier - The error along is a bit cryptic, and if you're seeing it, the solution is somewhat buried in docs: https://developers.cloudflare.com/durable-objects/best-practices/access-durable-objects-from-a-worker/#parse-previously-created-ids-from-strings chore(idFromString): make error message friendlier May 31, 2024
@thiskevinwang

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request May 31, 2024
- The error alone is a bit cryptic, and if you're seeing it, the solution is somewhat buried in docs: https://developers.cloudflare.com/durable-objects/best-practices/access-durable-objects-from-a-worker/#parse-previously-created-ids-from-strings
- This makes the `idFromString` error slightly friendlier
@thiskevinwang thiskevinwang force-pushed the durable-object-id-error-message branch from 75ec0aa to 95eb765 Compare May 31, 2024 03:37
@DaniFoldi

Copy link
Copy Markdown
Contributor

Hey 👋,

As someone who's run into this before, would it make sense to also mention jurisdictions in the error message? That also needs to match: https://developers.cloudflare.com/durable-objects/reference/data-location/#restrict-durable-objects-to-a-jurisdiction

@thiskevinwang

Copy link
Copy Markdown
Author

@DaniFoldi I'm not able to test/repro jurisdictions related errors locally with vitest-pool-workers. I run into: Error: Jurisdiction restrictions are not implemented in workerd..

Do the same requirements of having to first call newUniqueId() or idFromName() on the subnamespace apply?

I don't have strong opinions on including/omitting jurisdiction mention. It is also a namespace, so the current error message is still accurate w.r.t jurisdictions 😄

@jasnell jasnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm good with the change in general but I'm not sure it really provides much additional help.

JSG_REQUIRE(memcmp(id + BASE_LENGTH, decoded.begin() + BASE_LENGTH,
decoded.size() - BASE_LENGTH) == 0,
TypeError, "Durable Object ID is not valid for this namespace.");
TypeError, "Durable Object ID is not valid for this namespace. Please ensure that the ID was previously created within the same namespace.");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I'm not sure this really adds information. Maybe we can rephrase to something like:

Durable Object ID did not come from this namespace.

?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@kentonv Happy to rephrase it to anything else!

The main idea behind the additional error text is to provide remediation steps during testing, which is the angle I am/was coming from.

For context into what I was doing:

  • I was using a "fake" Durable Object ID in a test, like so this, and hitting the error, which I was not aware how to resolve.
  • I eventually stumbled my way through the docs and found out I had to call newUniqueId() or idFromName() in my tests first.
    • This additional trip through the docs is something I'm hoping the error message itself can help skip.

Ideas for rephrasing:

  • Maybe the error text should mention those methods directly?
  • Maybe the error text can link to docs? (This is a great DX, but it's an additional SDK-to-Web contract to maintain 😅 )

WDYT?

@kentonv

kentonv commented May 31, 2024

Copy link
Copy Markdown
Member

@DaniFoldi I don't think this particular error can be the result of using the wrong jurisdiction. If you use the wrong jurisdiction you'll get an ID that's valid for the namespace but points to a different object.

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.

4 participants