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

Guide to overriding sugar methods #763

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

hacksparrow
Copy link
Member

Guide explaining sugar methods and how to override them; resolves #757.

@dougwilson @crandmck @raijinsetsu

@crandmck
Copy link
Member

I may have some small wording improvements, but I'll wait first to see if there are any technical review comments.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

One technical-level comment I had :)

en/guide/overriding-sugar-methods.md Outdated Show resolved Hide resolved
@crandmck
Copy link
Member

crandmck commented Jan 11, 2017

Now that I read this more closely, I have a couple of more substantive questions/comments...

Sugar methods ... make up the bulk of Express' documented methods.

This begs the question of exactly which methods are sugar methods that you can override? Is it actually all of them? Only a subset? ...if so, we should clearly list them. Are there any that you shouldn't (even if you technically could?) Are there any side-effects of doing so to caveat? Rather than focus on what is technically possible, I think we should state "best practices" to the extent we know.

Also, although I think most developers are familiar with the concept of "syntactic sugar" I don't think the term "sugar methods" would be universally understood. I suggest instead the title should just be "Overriding Express methods" and then explain the in the text what a "sugar method" is and (per above) exactly which methods they are.

Also, it would be nice to give some idea of why you would want to do this, i.e. what you can accomplish by doing so.

@hacksparrow
Copy link
Member Author

This begs the question of exactly which methods are sugar methods that you can override? ...

The Express request and response objects extend the Node HTTP request object and response object respectively, and add methods such as req.accepts, req.param, res.send, res.status, and so on, which make for an easy and friendly API for the users to interact with the underlying Node methods and properties.

Technically, one can override all of the Express methods. They can even override Node HTTP objects' methods.

Overriding Express or Node methods has a very open-ended outcome, so it is not possible to state "best practices" or side-effects. It can range from "same behavior" to "completely broken" and everything possible in between. Basically, if someone decides to override the methods, they are on their own, and should know what they are doing.

One tip that should be given to the "overriders" is to look the source code of the methods, if they decide to override. That will give them a clear picture of what (else) is happening in the method.

Also, although I think most developers are familiar with the concept of "syntactic sugar"...

Yes, "Overriding Express methods" sounds better. Let's change it as suggested.

Also, it would be nice to give some idea of why you would want to do this ...

Reason, "because Express does not do it the way I want it". Eg: #757

@crandmck I hope you will recompose the content based on my input.

@hacksparrow
Copy link
Member Author

hacksparrow commented Jan 12, 2017

My PR was based on the discussion at expressjs/express#3151. After listening to the discussion in the recording, I think it makes sense to extend "sugar properties" as well. So the title would be now "Overriding the Express API" or something along that line.

@dougwilson app, the Express application instance's methods can be overriden too. Do we want to add it as well? Similarly, Router instances.

@crandmck I will add info about overriding properties and other possibilities in the next comment.

@hacksparrow
Copy link
Member Author

Documented properties in the Express API (limited to request and response in this write up) are either:

A. Inherited from the Node core

res.headersSent

There are many other core properties, only res.headersSent is documented on the website. I don't remember why it was decided to document only headersSent; maybe becaus of this - expressjs/express#2109.

B. Assigned properties

req.app, req.baseUrl, req.originalUrl, res.app, etc.

These properties are assigned on to the request and response objects in the context of the urrent request-response cycle by the router.

C. Defined as getters.

req.protocol, req.secure, req.ip, req.ips, req.subdomains, etc.

Since A and B are dynamically added by the router for each new request-response cycle, they cannot be overriden.

Properties under C are basically helper methods defined as getters, which return the values in the context of the current request-response cycle, so their behavior can be changed by re-assigning a new getter.

The following code rewrites how the value of the ip is to be derived. Now, it simply returns '999.999.999.999' (an invalid IP address).

Object.defineProperty(app.request, 'ip', {
  configurable: true,
  enumerable: true,
  get: function () { return '999.999.999.999'; }
});

The assignment operator has no affect on properties defined with getters. For example:

app.get('/', (req, res) => {
    req.ip = 'HELLO'
    console.log(req.ip)
  }
)

req.ip won't be "HELLO" in the code above, it will still be what the getter was programmed to return.

@hacksparrow
Copy link
Member Author

hacksparrow commented Jan 14, 2017

@expressjs/express-tc do you suggest documenting the ability to extend and override the methods and properties on the express, app, and router instances as well?

@crandmck
Copy link
Member

@hacksparrow Let's try to land this!

In TC, we discussed just adding minimal info to start, and then we can add more later if we want to. Can you please make any final changes and then request final review?

@hacksparrow
Copy link
Member Author

@crandmck I have updated the PR for final review.

@expressjs/express-tc

en/guide/overriding-express-api.md Outdated Show resolved Hide resolved
en/guide/overriding-express-api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

time for this to land. there was an issue recently where this was used, so good to have something to point to. rebased and fixed up for merge

@dougwilson dougwilson closed this in 7bc9163 Apr 1, 2020
@dougwilson dougwilson merged commit 7bc9163 into gh-pages Apr 1, 2020
@dougwilson dougwilson deleted the overriding-sugar-methods branch April 1, 2020 04:39
@github-pages github-pages bot temporarily deployed to github-pages April 1, 2020 04:40 Inactive
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.

Missing documentation for overriding sugar methods
3 participants