Skip to content
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

BREAKING(http): clean up module #3661

Merged
merged 23 commits into from Oct 13, 2023
Merged

Conversation

lino-levan
Copy link
Contributor

@lino-levan lino-levan commented Sep 20, 2023

Closes #3655 and closes #3646

@github-actions github-actions bot added the http label Sep 20, 2023
@lino-levan lino-levan marked this pull request as ready for review September 20, 2023 20:08
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I think prepending unstable_ instead of having an unstable folder is a bit tidier. Either way, seeing the http_ prefix being dropped from these files is cool!

http/http_errors.ts Outdated Show resolved Hide resolved
http/unstable/cookie_map_test.ts Outdated Show resolved Hide resolved
http/util.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come this is part of the unstable API? I'm sure I'm missing context, but the move surprises me. We use errors in SaaSKit to handle serving error responses in the REST API by throwing these HTTP errors (https://github.com/denoland/saaskit/blob/main/plugins/error_handling.ts).

Copy link
Member

Choose a reason for hiding this comment

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

In my view, representing http errors as JS Error objects is an opinionated approach.

In most cases when you throw http error in your route handler or middleware, you can also directly return Response object with error status code.

For example, the below code:

throw createHttpError(status, message)

can usually be replaced by

return new Response(message, { status });

or similar. If this replacement can be applied universally, then HttpError abstraction might be just redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If a request handler has a call stack size of n, using returns and finally returning an error response may require up to n conditionals. An error needs to be handled once to return an error response. I previously used the former method in SaaSKit before realising it could be done much easier once I discovered the HTTP error code in the Standard Library. It certainly made things easier to deal with.

To be clear, if we decide that HTTP errors should be unstable, fair enough. I just wanted to provide a valid counterargument.

Copy link
Member

Choose a reason for hiding this comment

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

That might be a valid use case. Let's keep the discussion in an independent issue

@kt3k
Copy link
Member

kt3k commented Sep 27, 2023

I think prepending unstable_ instead of having an unstable folder is a bit tidier.

Might be a good idea. @lino-levan What do you think?

BTW we already have unstable/ sub directory in crypto and collections

@lino-levan
Copy link
Contributor Author

I think we're at least a little committed to the unstable folder convention, but I think unstable_x reads better. The point of unstable APIs anyways is that they're little used so it'd probably be fine to move them around one more time.

@iuioiua
Copy link
Collaborator

iuioiua commented Oct 4, 2023

Gentle ping, @lino-levan.

@lino-levan
Copy link
Contributor Author

I have some concerns. Following up on discord.

@lino-levan
Copy link
Contributor Author

This should be ready to be merged now. Please review again, when you both get the chance.

http/http_errors.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Oct 11, 2023

Looks good to me. Thanks for your contribution! @lino-levan

Let's land when Asher's point is addressed.

@lino-levan
Copy link
Contributor Author

I believe this is ready to be merged. Please review. I would like a second pair of eyes to see if I re-exported everything properly.

@kt3k
Copy link
Member

kt3k commented Oct 13, 2023

It looks like the below structure doesn't work for deprecating re-export.

/** @deprecated */
export { foo } from "..";

Instead, we need to create aliases and deprecate them.

import { foo as foo_ } from "..";
/** @deprecated */
export const foo = foo_;

I'll make these changes in this branch

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution!

@kt3k kt3k merged commit 65125db into denoland:main Oct 13, 2023
12 checks passed
@lino-levan
Copy link
Contributor Author

Thanks for the final push @kt3k! Good to know what the proper reexport structure is.

@iuioiua
Copy link
Collaborator

iuioiua commented Oct 14, 2023

We should check whether other deprecated sub-modules have included the correct deprecation JSDocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up plan for http module Rename http_status.ts and http_errors.ts
3 participants