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

Fix up express() return type #19

Merged
merged 1 commit into from
Feb 17, 2017
Merged

Fix up express() return type #19

merged 1 commit into from
Feb 17, 2017

Conversation

blakeembrey
Copy link
Contributor

Modifying the prototype is part of middleware in Express, that's how it accepts raw HTTP.

Closes #18.

@felixfbecker I tried to make the minimal amount of changes, the description of why is in the linked repo 😄 If this makes sense, feel free to merge and update the registry, hoping to use these changes soon because I've been messing around with refactoring the underlying HTTP layer.

Modifying the prototype is part of middleware in Express, that's how it accepts raw HTTP.
@felixfbecker
Copy link
Collaborator

Are you sure that createApplication returns an interface that is different from Application? Can you point me to the source? I was under the impression that createApplication returns an Application, and the Application interface implements the HTTP server interface.

@blakeembrey
Copy link
Contributor Author

Yes, definitely. Express is designed to accept the raw HTTP objects and alters them to add Express.js-specifics. See https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/express.js#L37-L39 for the handler code, then https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/application.js#L157-L174 which is the .handle function (notice callback defaults to finalhandler). You'll notice none of this changes the req and res objects yet from raw HTTP, so you'll find this._router (aka lazyrouter) does https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/application.js#L144 which uses https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/middleware/init.js#L28-L29 (aka change the HTTP request and response prototypes to Express.js' request and response prototypes which internally extend the raw HTTP request and response prototypes anyway (see https://github.com/expressjs/express/blob/bd47aeb88d28c6cff6e2d8e5378ec73c262e039f/lib/request.js#L31).

@winksaville
Copy link

Is this going to merged into master?

@felixfbecker
Copy link
Collaborator

Sorry, forgot about this. @blakeembrey I assume the node typings are outdated by now again, but LGTM otherwise.

@blakeembrey blakeembrey merged commit 0b8501d into master Feb 17, 2017
@blakeembrey blakeembrey deleted the improve-server-types branch February 17, 2017 20:51
@blakeembrey
Copy link
Contributor Author

@felixfbecker Since it's a global and the types are used correctly, it won't be a problem.

@winksaville
Copy link

Txs for merging, I just installed it and then did:

$ typings info express
KEY         VALUE                   
name        express                 
source      npm                     
homepage                            
description                         
updated     2017-02-17T20:54:33.000Z
versions    1                       

And there is no homepage or description fields and the 'versions' is 1. I see in the package.json file there is a description and a version field but no homepage field:

{
  "name": "typed-express",
  "version": "1.0.0",
  "private": true,
  "description": "Typings for express",
  "scripts": {
    "build": "typings bundle -o test/bundle.d.ts",
    "test": "tsc -p test",
    "lint": "tslint -c tslint.json index.d.ts lib/**/*.d.ts test/**/*.ts"
  },
  "author": "Felix Becker <[email protected]>",
  "license": "ISC",
  "devDependencies": {
    "tslint": "^4.3.1",
    "typescript": "^2.1.5",
    "typings": "^2.1.0"
  }
}

So I'm a little confused, why is description empty?

Is info versions a count, it seems to be as typings info express --versions only show one:

$ typings info express --versions
TAG                   VERSION DESCRIPTION COMPILER LOCATION                                                          UPDATED                 
4.14.0+20170217205433 4.14.0                       github:types/npm-express#0b8501d546a03055e30749ac9d91fef351319af7 2017-02-17T20:54:33.000Z

But why is there only one version I would have expected there to be at least
two, is it because package.json version wasn't bumped, if so should the
package.json version field be bumped?

Should homepage be added to the package.json?

@blakeembrey
Copy link
Contributor Author

Those fields are defined on the registry itself. Usually reserved for information about the definition vs a regular description of the interface, but I don't think anyone uses it (except once). The versions listed from Typings is also all the current definitions in the registry, the old ones continue working but they are hidden behind a deprecated flag - it'd be a bit annoying seeing every change ever merged so we just show the currently listed registry versions.

@winksaville
Copy link

As a user how do I know if I have the latest version of a particular module and which repo it came from?

@blakeembrey
Copy link
Contributor Author

In the snippet you pasted above, see the location.

@winksaville
Copy link

Needing to use typings info express --version and then seeing a partial url for the location doesn't seem very friendly. It seems to me typings info express should be enough and then homepage, description and version should display information for the package which is what people would be interested in. If you wanted the "registry" maybe just typings info would display the registry information.

@blakeembrey
Copy link
Contributor Author

What if different versions are from different repos? What about showing the commit, which might be different across versions? I don't see the issue with the current approach considering most people don't really care where it's from and Typings is in maintenance mode since TypeScript has @types now. If anything, it might be worth printing the version information on the same screen as the rest to make it easier.

@winksaville
Copy link

I didn't realize Typings is in maintenance mode. Do you use npm @types/express or something else?

I ask because npm install @types/express index.d.ts is totally different from what is returned by typings install express

$ wc node_modules/@types/express/index.d.ts
  74  280 2562 node_modules/@types/express/index.d.ts
$ wc typings/modules/express/index.d.ts 
 1716  8092 65509 typings/modules/express/index.d.ts

@felixfbecker
Copy link
Collaborator

@winksaville
Copy link

winksaville commented Feb 18, 2017

@felixfbecker so it looks like someday, maybe sooner maybe later, this will be fixed :(

In the meantime which would you recommend I use, the versions of .d.ts files using typings install xxx or those from npm install @typs/xxx?

@blakeembrey
Copy link
Contributor Author

I'm still going to maintain Typings while people are using it, but I'm not going to promote it over the official solution TypeScript has created. See #14 for information on that point - we still can't publish anything to NPM unfortunately (which is why I haven't officially deprecated Typings yet).

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.

3 participants