Skip to content

[tables] Add RestError export#18635

Merged
joheredi merged 2 commits intoAzure:mainfrom
dhensby:tables/export-rest-error
Nov 11, 2021
Merged

[tables] Add RestError export#18635
joheredi merged 2 commits intoAzure:mainfrom
dhensby:tables/export-rest-error

Conversation

@dhensby
Copy link
Copy Markdown
Contributor

@dhensby dhensby commented Nov 11, 2021

This adds the RestError class that can be thrown by the library to the exports so that consumers can more easily catch and perform instanceof checks on thrown errors.

This is the convention with @azure/storage-blob and @azure/storage-queue

@dhensby dhensby requested a review from joheredi as a code owner November 11, 2021 00:44
@ghost ghost added Tables customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Nov 11, 2021
@ghost
Copy link
Copy Markdown

ghost commented Nov 11, 2021

Thank you for your contribution dhensby! We will review the pull request and get back to you soon.

@dhensby dhensby force-pushed the tables/export-rest-error branch from 3df769a to 184f6be Compare November 11, 2021 00:56
@ghost
Copy link
Copy Markdown

ghost commented Nov 11, 2021

CLA assistant check
All CLA requirements met.

@joheredi
Copy link
Copy Markdown
Member

Thanks for your contribution @dhensby. I'd like to understand a bit more about the context and scope of this fix, is there any scenarios where you find we should be throwing a RestError but we are not? Or is this change just meant to export the RestError to be referenced, without any intended changes in the SDK behavior?

@dhensby
Copy link
Copy Markdown
Contributor Author

dhensby commented Nov 11, 2021

Or is this change just meant to export the RestError to be referenced, without any intended changes in the SDK behavior?

That's absolutely right. It's so that the errors that are thrown can be caught and acted upon. For example:

const { TableServiceClient, RestError } = require('@azure/data-tables');

const client = new TableServiceClient(...);
client.createTable('mytable')
  .then(() => console.log('created table'))
  .catch((err) => {
    if (err intanceof RestError) {
      // some specific error handling
    }
  });

I have tried instead to import directly from the @azure/core-rest-pipeline library, but this is unreliable because of how npm can manage and de-dupe (or not) shared dependencies. So an approach like this that I have taken does not work in some instances:

const { TableServiceClient } = require('@azure/data-tables');
const { RestError } = require('@azure/core-rest-pipeline');

const client = new TableServiceClient(...);
client.createTable('mytable')
  .then(() => console.log('created table'))
  .catch((err) => {
    if (err intanceof RestError) { // this is fragile and will not work reliably
      // some specific error handling
    }
  });

This PR isn't intended to change any behavior, instead it just more clearly defines the contract between the package and the consumer.

@bterlson
Copy link
Copy Markdown
Member

This point about importing error types from core being unreliable is good. We could probably have a guideline around always ensuring every error a package might throw has exported its type. This is also why I generally prefer people checking error name and code, but TS 4.4 made this a little more difficult in strict mode unfortunately.

@dhensby
Copy link
Copy Markdown
Contributor Author

dhensby commented Nov 11, 2021

This is also why I generally prefer people checking error name and code

This is what I've done for the meantime, but as I posted originally, other @azure/* packages to export their respective RestError class, so it just felt natural this this library should too.

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Much like how we re-export credentials a SDK uses, I agree we should do the same for Error types.

Copy link
Copy Markdown
Member

@joheredi joheredi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @dhensby

@joheredi joheredi merged commit 6e2b3d1 into Azure:main Nov 11, 2021
@dhensby dhensby deleted the tables/export-rest-error branch November 12, 2021 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. Tables

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants