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

refactor: rename FreshContext.req to FreshContext.request #2593

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Jul 10, 2024

ref: #2363 (comment)

  • renames FreshContext .req property to FreshContext.request

@marvinhagemeister

@timreichen
Copy link
Contributor Author

@marvinhagemeister Ready for review.

Copy link
Collaborator

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

At Deno here we prefer ctx.req for brevity reasons. Merging this would make my coworkers unhappy. Instead of removing it entirely, you could add a getter to alias ctx.request -> ctx.req.

@timreichen
Copy link
Contributor Author

At Deno here we prefer ctx.req for brevity reasons. Merging this would make my coworkers unhappy. Instead of removing it entirely, you could add a getter to alias ctx.request -> ctx.req.

Are you sure? It seems there are lot of upvotes which agree with the change (#2363 (comment)
I would find it weird to have a property alias just for convenience in a new public API.

@Atmos4
Copy link

Atmos4 commented Sep 4, 2024

@timreichen I don't find req weird personally

Because let's be real, this PR is only nitpicking and personal preference. Deep down it's not important what the name of the variable is as long as it's somewhat clear.

I use Hono a lot, it's req in Hono, it saves both maintainers and lib users a few characters, everyone knows what it is, everyone is happy.

Apart from you apparently.

@timreichen
Copy link
Contributor Author

timreichen commented Sep 5, 2024

@timreichen I don't find req weird personally

Because let's be real, this PR is only nitpicking and personal preference. Deep down it's not important what the name of the variable is as long as it's somewhat clear.

I use Hono a lot, it's req in Hono, it saves both maintainers and lib users a few characters, everyone knows what it is, everyone is happy.

Apart from you apparently.

I don't find req weird per se, I would find it weird to have both request and req in an interface when they represent the exact same as proposed by @marvinhagemeister.

But this is not about personal preference, it is about consistency and alignment with web apis.
As I pointed out, request as a property is present in FetchEvent.prototype.request. Naming a property req instead request that represents a Request simply makes it inconsistent with web apis.

What I find would be a personal preference however is to sacrifice consistency over the convenience of saving a few chars.

@timreichen
Copy link
Contributor Author

@marvinhagemeister I think we should resolve this issue soon. I worked a lot on deno @std for the last two years helping to make the mods consistent among each other and follow web api syntax and conventions. So this issue seems like a no-brainer to me. Is there any chance it can be adopted?

@nnmrts
Copy link

nnmrts commented Oct 4, 2024

Apart from the consistency concerns, I think "req" instead of "request" is also bad because:

  • Other, especially older frameworks doing the same does not mean fresh should do it. Quite the opposite actually: future developers born today will not grow up scrolling through express.js docs like we did, so it becomes increasingly likely they are not familiar with the usual req, res or reg, ctx scheme.
  • Without enough context (and not "ctx" 😜) it could be confused with the common "require", "requirement" or for people with bad eyes (like me) even with words starting with "reg" like "register", "regular" or "region".
  • Without it being the full word, it's unclear which form of the word it is: "request", "requests", "requesting", "requested", etc. are all common variations of that word, affecting the understanding of the actual format of the value behind the variable. Is it an array of requests, or maybe even a boolean?
  • It sets a precedent in code bases; let's also call it "res" instead of "response" (which a lot of frameworks sadly do as well, I know), now "res" could also mean the common words "resource", "resolution", "reserve" or even "research". And why not call it "dep" instead of "dependency" (also common, sadly), which could stand for a bunch of common words too?
  • There are abbreviations which actually save time typing and maybe even reading and understanding code, for example "fs" instead of "file system" or the more cryptic "i18n" instead of "internationalization", but in this case, it's 3 instead of 7 letters. We are not saving any "real" time here, and also, by the time you typed the "q", your (hopefully modern) IDE will autosuggest or -complete it to "request" anyway.
  • "Using complete words results in more readable code. Not everyone knows all your abbreviations. Code is written only once, but read many times." - quoted from a documentation of an eslint rule I personally use (with great success 😉).

In many of the points above, I speak from personal experience. On several occasions, I’ve had to remind work colleagues, non-technical product owners, or even my own tired 3am brain what "req", "res", "opt", "dep", "acc", etc. mean at some line deep in a codebase. And that is where you lose actual time. In our newer codebases, I introduced the aforementioned eslint plugin and code reviews are now 20% more fun (a made up stat, but I hope you get my point).

@timreichen
Copy link
Contributor Author

@marvinhagemeister I added the alias with a deprecation note. I think that way a slow adoption can happen during the alpha releases and before 1.0 is released. WDYT?

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